{"thread":{"id":"47975","subject":"[PATCH 1/2] git-svn: search --authors-prog in PATH too","startedAt":"2018-03-04T11:31:28Z","lastAt":"2018-03-06T22:24:58Z","messageCount":11,"participants":["Andreas Heiduk","Eric Sunshine","Eric Wong"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"340979","messageId":"20180304112237.19254-1-asheiduk@gmail.com","threadId":"47975","inReplyTo":null,"subject":"[PATCH 1/2] git-svn: search --authors-prog in PATH too","fromName":"Andreas Heiduk","fromEmail":"asheiduk@gmail.com","sentAt":"2018-03-04T11:22:36Z","receivedAt":"2018-03-04T11:31:28Z","isPatch":true,"sender":{"key":"asheiduk@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9371344?v=4"},"body":"In 36db1eddf9 (\"git-svn: add --authors-prog option\", 2009-05-14) the path\nto authors-prog was made absolute because git-svn changes the current\ndirectoy in some situations. This makes sense if the program is part of\nthe repository but prevents searching via $PATH.\n\nThe old behaviour is still retained, but if the file does not exists, then\nauthors-prog is search in $PATH as any other command.\n\nSigned-off-by: Andreas Heiduk \n---\n Documentation/git-svn.txt | 5 +++++\n git-svn.perl | 3 ++-\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-svn.txt b/Documentation/git-svn.txt\nindex 636e09048e..b858374649 100644\n--- a/Documentation/git-svn.txt\n+++ b/Documentation/git-svn.txt\n@@ -657,6 +657,11 @@ config key: svn.authorsfile\n \texpected to return a single line of the form \"Name \",\n \twhich will be treated as if included in the authors file.\n +\n+Due to historical reasons a relative 'filename' is first searched\n+relative to the current directory for 'init' and 'clone' and relative\n+to the root of the working tree for 'fetch'. If 'filename' is\n+not found, it is searched like any other command in '$PATH'.\n++\n [verse]\n config key: svn.authorsProg\n \ndiff --git a/git-svn.perl b/git-svn.perl\nindex a6b6c3e40c..050f2a36f4 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -374,7 +374,8 @@ version() if $_version;\n usage(1) unless defined $cmd;\n load_authors() if $_authors;\n if (defined $_authors_prog) {\n-\t$_authors_prog = \"'\" . File::Spec->rel2abs($_authors_prog) . \"'\";\n+\tmy $abs_file = File::Spec->rel2abs($_authors_prog);\n+\t$_authors_prog = \"'\" . $abs_file . \"'\" if -x $abs_file;\n }\n \n unless ($cmd =~ /^(?:clone|init|multi-init|commit-diff)$/) {\n-- \n2.16.2\n\n"},{"id":"340980","messageId":"20180304112237.19254-2-asheiduk@gmail.com","threadId":"47975","inReplyTo":"20180304112237.19254-1-asheiduk@gmail.com","subject":"[PATCH 2/2] git-svn: allow empty email-address in authors-prog and authors-file","fromName":"Andreas Heiduk","fromEmail":"asheiduk@gmail.com","sentAt":"2018-03-04T11:22:37Z","receivedAt":"2018-03-04T11:31:30Z","isPatch":true,"sender":{"key":"asheiduk@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9371344?v=4"},"body":"The email address in --authors-file and --authors-prog can be empty but\ngit-svn translated it into a syntethic email address in the form\n$USERNAME@$REPO_UUID. Now git-svn behaves like git-commit: If the email\nis explicitly set to the empty string, the commit does not contain\nan email address.\n\nSigned-off-by: Andreas Heiduk \n---\n perl/Git/SVN.pm | 13 ++++++-------\n 1 file changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm\nindex bc4eed3d75..b0a340b294 100644\n--- a/perl/Git/SVN.pm\n+++ b/perl/Git/SVN.pm\n@@ -1482,7 +1482,6 @@ sub call_authors_prog {\n \t}\n \tif ($author =~ /^\\s*(.+?)\\s*<(.*)>\\s*$/) {\n \t\tmy ($name, $email) = ($1, $2);\n-\t\t$email = undef if length $2 == 0;\n \t\treturn [$name, $email];\n \t} else {\n \t\tdie \"Author: $orig_author: $::_authors_prog returned \"\n@@ -2020,8 +2019,8 @@ sub make_log_entry {\n \t\tremove_username($full_url);\n \t\t$log_entry{metadata} = \"$full_url\\@$r $uuid\";\n \t\t$log_entry{svm_revision} = $r;\n-\t\t$email ||= \"$author\\@$uuid\";\n-\t\t$commit_email ||= \"$author\\@$uuid\";\n+\t\t$email //= \"$author\\@$uuid\";\n+\t\t$commit_email //= \"$author\\@$uuid\";\n \t} elsif ($self->use_svnsync_props) {\n \t\tmy $full_url = canonicalize_url(\n \t\t\tadd_path_to_url( $self->svnsync->{url}, $self->path )\n@@ -2029,15 +2028,15 @@ sub make_log_entry {\n \t\tremove_username($full_url);\n \t\tmy $uuid = $self->svnsync->{uuid};\n \t\t$log_entry{metadata} = \"$full_url\\@$rev $uuid\";\n-\t\t$email ||= \"$author\\@$uuid\";\n-\t\t$commit_email ||= \"$author\\@$uuid\";\n+\t\t$email //= \"$author\\@$uuid\";\n+\t\t$commit_email //= \"$author\\@$uuid\";\n \t} else {\n \t\tmy $url = $self->metadata_url;\n \t\tremove_username($url);\n \t\tmy $uuid = $self->rewrite_uuid || $self->ra->get_uuid;\n \t\t$log_entry{metadata} = \"$url\\@$rev \" . $uuid;\n-\t\t$email ||= \"$author\\@\" . $uuid;\n-\t\t$commit_email ||= \"$author\\@\" . $uuid;\n+\t\t$email //= \"$author\\@\" . $uuid;\n+\t\t$commit_email //= \"$author\\@\" . $uuid;\n \t}\n \t$log_entry{name} = $name;\n \t$log_entry{email} = $email;\n-- \n2.16.2\n\n"},{"id":"340994","messageId":"CAPig+cQARx4eZFHyRpikHJYaz42NKq18fjt-=KYOX2GddDPNDA@mail.gmail.com","threadId":"47975","inReplyTo":"20180304112237.19254-1-asheiduk@gmail.com","subject":"Re: [PATCH 1/2] git-svn: search --authors-prog in PATH too","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-05T00:52:32Z","receivedAt":"2018-03-05T00:52:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 4, 2018 at 6:22 AM, Andreas Heiduk wrote:\n> In 36db1eddf9 (\"git-svn: add --authors-prog option\", 2009-05-14) the path\n> to authors-prog was made absolute because git-svn changes the current\n> directoy in some situations. This makes sense if the program is part of\n\ns/directoy/directory/\n\n> the repository but prevents searching via $PATH.\n>\n> The old behaviour is still retained, but if the file does not exists, then\n> authors-prog is search in $PATH as any other command.\n\ns/search/searched for/\n\n> Signed-off-by: Andreas Heiduk \n"},{"id":"340995","messageId":"CAPig+cQTpbj6q_s_pX2HjmpYF4W+5wKH7YLQQdZTiMPfK4G2Ng@mail.gmail.com","threadId":"47975","inReplyTo":"20180304112237.19254-2-asheiduk@gmail.com","subject":"Re: [PATCH 2/2] git-svn: allow empty email-address in authors-prog and authors-file","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-05T01:42:35Z","receivedAt":"2018-03-05T01:42:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 4, 2018 at 6:22 AM, Andreas Heiduk wrote:\n> The email address in --authors-file and --authors-prog can be empty but\n> git-svn translated it into a syntethic email address in the form\n> $USERNAME@$REPO_UUID. Now git-svn behaves like git-commit: If the email\n> is explicitly set to the empty string, the commit does not contain\n> an email address.\n\nFalling back to \"$name@$uuid\" was clearly an intentional choice, so\nthis seems like a rather significant change of behavior. How likely is\nit that users or scripts relying upon the existing behavior will break\nwith this change? If the likelihood is high, should this behavior be\nopt-in?\n\nDoesn't such a behavior change deserve being documented (and possibly tests)?\n\n> Signed-off-by: Andreas Heiduk \n> ---\n> diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm\n> @@ -1482,7 +1482,6 @@ sub call_authors_prog {\n> }\n> if ($author =~ /^\\s*(.+?)\\s*<(.*)>\\s*$/) {\n> my ($name, $email) = ($1, $2);\n> - $email = undef if length $2 == 0;\n> return [$name, $email];\n\nMental note: existing behavior intentionally makes $email undefined if\nnot present in $author; revised behavior leaves it defined.\n\n> } else {\n> die \"Author: $orig_author: $::_authors_prog returned \"\n> @@ -2020,8 +2019,8 @@ sub make_log_entry {\n> remove_username($full_url);\n> $log_entry{metadata} = \"$full_url\\@$r $uuid\";\n> $log_entry{svm_revision} = $r;\n> - $email ||= \"$author\\@$uuid\";\n> - $commit_email ||= \"$author\\@$uuid\";\n> + $email //= \"$author\\@$uuid\";\n> + $commit_email //= \"$author\\@$uuid\";\n\nWith the revised behavior (above), $email is unconditionally defined,\nso these //= expressions will _never_ assign \"$author\\@$uuid\" to\n$email. Am I reading that correctly? If so, then isn't this now just\ndead code? Wouldn't it be clearer to remove these lines altogether?\n\nI see from reading the code that there is a \"if (!defined $email)\"\nearlier in the function which becomes misleading with this change. I'd\nhave expected the patch to modify that, as well.\n\nAlso, the Perl codebase in this project is still at 5.8, whereas the\n// operator (and //=) didn't become available until Perl 5.10.\n(However, there has lately been some talk[1] about bumping the minimum\nPerl version to 5.10.)\n\n[1]: https://public-inbox.org/git/20171223174400.26668-1-avarab@gmail.com/\n\n> } elsif ($self->use_svnsync_props) {\n> my $full_url = canonicalize_url(\n> add_path_to_url( $self->svnsync->{url}, $self->path )\n> @@ -2029,15 +2028,15 @@ sub make_log_entry {\n> remove_username($full_url);\n> my $uuid = $self->svnsync->{uuid};\n> $log_entry{metadata} = \"$full_url\\@$rev $uuid\";\n> - $email ||= \"$author\\@$uuid\";\n> - $commit_email ||= \"$author\\@$uuid\";\n> + $email //= \"$author\\@$uuid\";\n> + $commit_email //= \"$author\\@$uuid\";\n> } else {\n> my $url = $self->metadata_url;\n> remove_username($url);\n> my $uuid = $self->rewrite_uuid || $self->ra->get_uuid;\n> $log_entry{metadata} = \"$url\\@$rev \" . $uuid;\n> - $email ||= \"$author\\@\" . $uuid;\n> - $commit_email ||= \"$author\\@\" . $uuid;\n> + $email //= \"$author\\@\" . $uuid;\n> + $commit_email //= \"$author\\@\" . $uuid;\n> }\n> $log_entry{name} = $name;\n> $log_entry{email} = $email;\n"},{"id":"341002","messageId":"CAFhHFBysKuDO9H4yJtnC6MJ+Jih5q4RsfwHTCsRXhXknapp4xg@mail.gmail.com","threadId":"47975","inReplyTo":"CAPig+cQTpbj6q_s_pX2HjmpYF4W+5wKH7YLQQdZTiMPfK4G2Ng@mail.gmail.com","subject":"Re: [PATCH 2/2] git-svn: allow empty email-address in authors-prog and authors-file","fromName":"Andreas Heiduk","fromEmail":"asheiduk@gmail.com","sentAt":"2018-03-05T09:37:59Z","receivedAt":"2018-03-05T09:38:06Z","isPatch":true,"sender":{"key":"asheiduk@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9371344?v=4"},"body":"2018-03-05 2:42 GMT+01:00 Eric Sunshine :\n> On Sun, Mar 4, 2018 at 6:22 AM, Andreas Heiduk wrote:\n>> The email address in --authors-file and --authors-prog can be empty but\n>> git-svn translated it into a syntethic email address in the form\n>> $USERNAME@$REPO_UUID. Now git-svn behaves like git-commit: If the email\n>> is explicitly set to the empty string, the commit does not contain\n>> an email address.\n>\n> Falling back to \"$name@$uuid\" was clearly an intentional choice, so\n> this seems like a rather significant change of behavior. How likely is\n> it that users or scripts relying upon the existing behavior will break\n> with this change? If the likelihood is high, should this behavior be\n> opt-in?\n\nI don't know nor understand the intial choice. Didn' git-commit support\nempty emails once upon a time? Or is the SVN-UID important for some\nSVK/SVM workflows?\n\nMy reasoning was: If authors-prog is NOT used, the behaviour\nis unchanged - the UUID will be generated. If a Script IS used, then I\nassume that a valid email was generated and this change will not\nbreak these scripts. Only scripts intentionally not generating emails\nwill break. But again - was would be the purpose of such a script?\nAnd such a script can be easily changed to add the UUID again.\n\nSo I think adding an explicit opt-in does not pay off.\n\n\n> Doesn't such a behavior change deserve being documented (and possibly tests)?\n\nThe old behaviour was neither documented nor tested - the\nchange did not break any test after all.\n\n>\n>> Signed-off-by: Andreas Heiduk \n>> ---\n>> diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm\n>> @@ -1482,7 +1482,6 @@ sub call_authors_prog {\n>> }\n>> if ($author =~ /^\\s*(.+?)\\s*<(.*)>\\s*$/) {\n>> my ($name, $email) = ($1, $2);\n>> - $email = undef if length $2 == 0;\n>> return [$name, $email];\n>\n> Mental note: existing behavior intentionally makes $email undefined if\n> not present in $author; revised behavior leaves it defined.\n\nBut only if the data comes from authors-prog. authors-file is unaffected.\n\n>\n>> } else {\n>> die \"Author: $orig_author: $::_authors_prog returned \"\n>> @@ -2020,8 +2019,8 @@ sub make_log_entry {\n>> remove_username($full_url);\n>> $log_entry{metadata} = \"$full_url\\@$r $uuid\";\n>> $log_entry{svm_revision} = $r;\n>> - $email ||= \"$author\\@$uuid\";\n>> - $commit_email ||= \"$author\\@$uuid\";\n>> + $email //= \"$author\\@$uuid\";\n>> + $commit_email //= \"$author\\@$uuid\";\n>\n> With the revised behavior (above), $email is unconditionally defined,\n> so these //= expressions will _never_ assign \"$author\\@$uuid\" to\n> $email. Am I reading that correctly? If so, then isn't this now just\n> dead code? Wouldn't it be clearer to remove these lines altogether?\n\nThe olf behaviour still kicks in if\n - neither authors-file nor authors-prog is used\n - only authors-file is used\n\n> I see from reading the code that there is a \"if (!defined $email)\"\n> earlier in the function which becomes misleading with this change. I'd\n> have expected the patch to modify that, as well.\n\nI will look into that one later.\n\n> Also, the Perl codebase in this project is still at 5.8, whereas the\n> // operator (and //=) didn't become available until Perl 5.10.\n\nI can expand these into something like\n\n $email = defined $email ? $email : \"$author\\@$uuid\";\n\nor\n\n $email = \"$author\\@$uuid\" unless defined $email;\n\n\n> (However, there has lately been some talk[1] about bumping the minimum\n> Perl version to 5.10.)\n>\n> [1]: https://public-inbox.org/git/20171223174400.26668-1-avarab@gmail.com/\n\nDid the thread result in some actual action?\n\n\n>> } elsif ($self->use_svnsync_props) {\n>> my $full_url = canonicalize_url(\n>> add_path_to_url( $self->svnsync->{url}, $self->path )\n>> @@ -2029,15 +2028,15 @@ sub make_log_entry {\n>> remove_username($full_url);\n>> my $uuid = $self->svnsync->{uuid};\n>> $log_entry{metadata} = \"$full_url\\@$rev $uuid\";\n>> - $email ||= \"$author\\@$uuid\";\n>> - $commit_email ||= \"$author\\@$uuid\";\n>> + $email //= \"$author\\@$uuid\";\n>> + $commit_email //= \"$author\\@$uuid\";\n>> } else {\n>> my $url = $self->metadata_url;\n>> remove_username($url);\n>> my $uuid = $self->rewrite_uuid || $self->ra->get_uuid;\n>> $log_entry{metadata} = \"$url\\@$rev \" . $uuid;\n>> - $email ||= \"$author\\@\" . $uuid;\n>> - $commit_email ||= \"$author\\@\" . $uuid;\n>> + $email //= \"$author\\@\" . $uuid;\n>> + $commit_email //= \"$author\\@\" . $uuid;\n>> }\n>> $log_entry{name} = $name;\n>> $log_entry{email} = $email;\n"},{"id":"341037","messageId":"20180305175243.GA10325@dcvr","threadId":"47975","inReplyTo":"CAPig+cQARx4eZFHyRpikHJYaz42NKq18fjt-=KYOX2GddDPNDA@mail.gmail.com","subject":"Re: [PATCH 1/2] git-svn: search --authors-prog in PATH too","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2018-03-05T17:52:43Z","receivedAt":"2018-03-05T17:52:48Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Eric Sunshine wrote:\n> On Sun, Mar 4, 2018 at 6:22 AM, Andreas Heiduk wrote:\n> > In 36db1eddf9 (\"git-svn: add --authors-prog option\", 2009-05-14) the path\n> > to authors-prog was made absolute because git-svn changes the current\n> > directoy in some situations. This makes sense if the program is part of\n> \n> s/directoy/directory/\n> \n> > the repository but prevents searching via $PATH.\n> >\n> > The old behaviour is still retained, but if the file does not exists, then\n> > authors-prog is search in $PATH as any other command.\n> \n> s/search/searched for/\n> \n> > Signed-off-by: Andreas Heiduk \n\nThanks both, I've queued 1/2 up with Eric S's edits at svn/authors-prog.\nI'm not yet convinced 2/2 is a good change, however.\n\nThe following changes since commit 7e31236f652ad9db221511eaf157ce0ef55585d6:\n\n Sixth batch for 2.17 (2018-02-28 13:39:24 -0800)\n\nare available in the Git repository at:\n\n git://bogomips.org/git-svn.git svn/authors-prog\n\nfor you to fetch changes up to e99652c412701cf988770e5cfaa253712a39221a:\n\n git-svn: search --authors-prog in PATH too (2018-03-05 02:09:57 +0000)\n\n----------------------------------------------------------------\nAndreas Heiduk (1):\n git-svn: search --authors-prog in PATH too\n\n Documentation/git-svn.txt | 5 +++++\n git-svn.perl | 3 ++-\n 2 files changed, 7 insertions(+), 1 deletion(-)\n"},{"id":"341055","messageId":"7767e8ea-5a4b-5088-47e3-f9f2eb7ffae9@gmail.com","threadId":"47975","inReplyTo":"20180305175243.GA10325@dcvr","subject":"Re: [PATCH 1/2] git-svn: search --authors-prog in PATH too","fromName":"Andreas Heiduk","fromEmail":"asheiduk@gmail.com","sentAt":"2018-03-05T19:48:09Z","receivedAt":"2018-03-05T19:48:16Z","isPatch":true,"sender":{"key":"asheiduk@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9371344?v=4"},"body":"\n\nAm 05.03.2018 um 18:52 schrieb Eric Wong:\n> \n> Thanks both, I've queued 1/2 up with Eric S's edits at svn/authors-prog.\n> I'm not yet convinced 2/2 is a good change, however.\n\nI'm not sure which direction your argument points to: Do you object to a\n$PATH search at all? Or would you like to remove the legacy code too?\nOr is the code/doc/... in bad shape?\n"},{"id":"341057","messageId":"a767150a-cb33-5aad-68ed-27ef237ee9aa@gmail.com","threadId":"47975","inReplyTo":"7767e8ea-5a4b-5088-47e3-f9f2eb7ffae9@gmail.com","subject":"Re: [PATCH 1/2] git-svn: search --authors-prog in PATH too","fromName":"Andreas Heiduk","fromEmail":"asheiduk@gmail.com","sentAt":"2018-03-05T20:16:13Z","receivedAt":"2018-03-05T20:16:21Z","isPatch":true,"sender":{"key":"asheiduk@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9371344?v=4"},"body":"Am 05.03.2018 um 20:48 schrieb Andreas Heiduk:\n> Am 05.03.2018 um 18:52 schrieb Eric Wong:\n>> Thanks both, I've queued 1/2 up with Eric S's edits at svn/authors-prog.\n>> I'm not yet convinced 2/2 is a good change, however.\n> \n> I'm not sure which direction your argument points to: Do you object to a\n> $PATH search at all? Or would you like to remove the legacy code too?\n> Or is the code/doc/... in bad shape?\n\nOK, I've mismatched your reply and the patch number. So my new questions\nare:\n\nDo you object to the feature as such? Or would you like to remove the\nlegacy code too? Or is the code/doc/... in bad shape beyond what E.S.\nalready mentioned?\n\nRight now my team performs a hefty \"git filter-branch\" after each \"git\nsvn fetch\" followed by black magic to set the new object ids in the\ncaches / rev-maps of git-svn. Official support for \"no synthetic email\"\nwould be much easier.\n\n\n"},{"id":"341058","messageId":"20180305202017.GA26533@whir","threadId":"47975","inReplyTo":"CAFhHFBysKuDO9H4yJtnC6MJ+Jih5q4RsfwHTCsRXhXknapp4xg@mail.gmail.com","subject":"Re: [PATCH 2/2] git-svn: allow empty email-address in authors-prog and authors-file","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2018-03-05T20:20:17Z","receivedAt":"2018-03-05T20:20:22Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Andreas Heiduk wrote:\n> 2018-03-05 2:42 GMT+01:00 Eric Sunshine :\n> > Doesn't such a behavior change deserve being documented (and possibly tests)?\n> \n> The old behaviour was neither documented nor tested - the\n> change did not break any test after all.\n\nI consider that too low of a bar to justify a behavior change\nwhich can negatively affect users.\n"},{"id":"341077","messageId":"CAPig+cSRF0g_+rq_Ga-kg+fh2XnUobVKodkR9Z=pVskq7vRU6w@mail.gmail.com","threadId":"47975","inReplyTo":"CAFhHFBysKuDO9H4yJtnC6MJ+Jih5q4RsfwHTCsRXhXknapp4xg@mail.gmail.com","subject":"Re: [PATCH 2/2] git-svn: allow empty email-address in authors-prog and authors-file","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-05T22:22:45Z","receivedAt":"2018-03-05T22:22:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 5, 2018 at 4:37 AM, Andreas Heiduk wrote:\n> 2018-03-05 2:42 GMT+01:00 Eric Sunshine :\n>> On Sun, Mar 4, 2018 at 6:22 AM, Andreas Heiduk wrote:\n>>> The email address in --authors-file and --authors-prog can be empty but\n>>> git-svn translated it into a syntethic email address in the form\n>>> $USERNAME@$REPO_UUID. Now git-svn behaves like git-commit: If the email\n>>> is explicitly set to the empty string, the commit does not contain\n>>> an email address.\n>>\n>> Falling back to \"$name@$uuid\" was clearly an intentional choice, so\n>> this seems like a rather significant change of behavior. How likely is\n>> it that users or scripts relying upon the existing behavior will break\n>> with this change? If the likelihood is high, should this behavior be\n>> opt-in?\n>\n> I don't know nor understand the intial choice. Didn' git-commit support\n> empty emails once upon a time? Or is the SVN-UID important for some\n> SVK/SVM workflows?\n\nI don't know the answer to either question. As I've only ever used\ngit-svn a few times many years ago, I can't speak of the reason behind\nthe name@uuid fallback, but that it was intentional suggests that\nthere may have been a good reason for it.\n\n> My reasoning was: If authors-prog is NOT used, the behaviour\n> is unchanged - the UUID will be generated. If a Script IS used, then I\n> assume that a valid email was generated and this change will not\n> break these scripts. Only scripts intentionally not generating emails\n> will break. But again - was would be the purpose of such a script?\n> And such a script can be easily changed to add the UUID again.\n\nAs I'm not the author of every script in the wild, I can't answer as\nto the purpose of a script working in such a way, but that there may\nbe such scripts makes me cautious. We don't know how easy it would be\nfor a script to be \"fixed\" for this new behavior or even how soon the\n\"breakage\" would be noticed for scripts which have been working\nproperly and quietly in the background for years.\n\n> So I think adding an explicit opt-in does not pay off.\n\nI defer judgment to Eric W.'s area expertise.\n\n>>> if ($author =~ /^\\s*(.+?)\\s*<(.*)>\\s*$/) {\n>>> my ($name, $email) = ($1, $2);\n>>> - $email = undef if length $2 == 0;\n>>> return [$name, $email];\n>>\n>> Mental note: existing behavior intentionally makes $email undefined if\n>> not present in $author; revised behavior leaves it defined.\n>\n> But only if the data comes from authors-prog. authors-file is unaffected.\n>\n>>\n>>> } else {\n>>> die \"Author: $orig_author: $::_authors_prog returned \"\n>>> @@ -2020,8 +2019,8 @@ sub make_log_entry {\n>>> - $email ||= \"$author\\@$uuid\";\n>>> - $commit_email ||= \"$author\\@$uuid\";\n>>> + $email //= \"$author\\@$uuid\";\n>>> + $commit_email //= \"$author\\@$uuid\";\n>>\n>> With the revised behavior (above), $email is unconditionally defined,\n>> so these //= expressions will _never_ assign \"$author\\@$uuid\" to\n>> $email. Am I reading that correctly? If so, then isn't this now just\n>> dead code? Wouldn't it be clearer to remove these lines altogether?\n>\n> The olf behaviour still kicks in if\n> - neither authors-file nor authors-prog is used\n> - only authors-file is used\n\nOkay.\n\n>> (However, there has lately been some talk[1] about bumping the minimum\n>> Perl version to 5.10.)\n>>\n>> [1]: https://public-inbox.org/git/20171223174400.26668-1-avarab@gmail.com/\n>\n> Did the thread result in some actual action?\n\nNot to my knowledge. Scanning the thread, it looks like the issue is\nstill hanging.\n"},{"id":"341165","messageId":"426e019a-cfb2-0e3a-b9b4-9d94f4f79312@gmail.com","threadId":"47975","inReplyTo":"CAFhHFBysKuDO9H4yJtnC6MJ+Jih5q4RsfwHTCsRXhXknapp4xg@mail.gmail.com","subject":"Re: [PATCH 2/2] git-svn: allow empty email-address in authors-prog and authors-file","fromName":"Andreas Heiduk","fromEmail":"asheiduk@gmail.com","sentAt":"2018-03-06T22:24:50Z","receivedAt":"2018-03-06T22:24:58Z","isPatch":true,"sender":{"key":"asheiduk@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9371344?v=4"},"body":"Am 05.03.2018 um 10:37 schrieb Andreas Heiduk:\n> 2018-03-05 2:42 GMT+01:00 Eric Sunshine :\n>> On Sun, Mar 4, 2018 at 6:22 AM, Andreas Heiduk wrote:\n>>> ---\n>>> diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm\n>>> @@ -1482,7 +1482,6 @@ sub call_authors_prog {\n>>> }\n>>> if ($author =~ /^\\s*(.+?)\\s*<(.*)>\\s*$/) {\n>>> my ($name, $email) = ($1, $2);\n>>> - $email = undef if length $2 == 0;\n>>> return [$name, $email];\n>>\n>> Mental note: existing behavior intentionally makes $email undefined if\n>> not present in $author; revised behavior leaves it defined.\n> \n> But only if the data comes from authors-prog. authors-file is unaffected.\n> \n>>\n>>> } else {\n>>> die \"Author: $orig_author: $::_authors_prog returned \"\n>>> @@ -2020,8 +2019,8 @@ sub make_log_entry {\n>>> remove_username($full_url);\n>>> $log_entry{metadata} = \"$full_url\\@$r $uuid\";\n>>> $log_entry{svm_revision} = $r;\n>>> - $email ||= \"$author\\@$uuid\";\n>>> - $commit_email ||= \"$author\\@$uuid\";\n>>> + $email //= \"$author\\@$uuid\";\n>>> + $commit_email //= \"$author\\@$uuid\";\n>>\n>> With the revised behavior (above), $email is unconditionally defined,\n>> so these //= expressions will _never_ assign \"$author\\@$uuid\" to\n>> $email. Am I reading that correctly? If so, then isn't this now just\n>> dead code? Wouldn't it be clearer to remove these lines altogether?\n> \n> The olf behaviour still kicks in if\n> - neither authors-file nor authors-prog is used\n> - only authors-file is used\n> \n>> I see from reading the code that there is a \"if (!defined $email)\"\n>> earlier in the function which becomes misleading with this change. I'd\n>> have expected the patch to modify that, as well.\n> \n> I will look into that one later.\n\nI don't want to let that slip through the cracks: The `if` statement\nstill makes a difference if:\n\n - neither ` --authors-prog` nor `--authors-file` is active,\n - but `--use-log-author` is active and\n - the commit at hand does not contain a `From:` or `Signed-off-by:`\n trailer.\n\nIn that case the result was and still is `$user <$user>` for the author\nand `$user <$user@$uuid>` for the comitter. That doesn't make sense to\nme but doesn't concern me right now.\n"}]}