[PATCH 1/2] git-svn: search --authors-prog in PATH too

[PATCH 1/2] git-svn: search --authors-prog in PATH too

[Andreas Heiduk]

In 36db1eddf9 ("git-svn: add --authors-prog option", 2009-05-14) the path
to authors-prog was made absolute because git-svn changes the current
directoy in some situations. This makes sense if the program is part of
the repository but prevents searching via $PATH.

The old behaviour is still retained, but if the file does not exists, then
authors-prog is search in $PATH as any other command.

Signed-off-by: Andreas Heiduk <asheiduk@gmail.com>
---
 Documentation/git-svn.txt | 5 +++++
 git-svn.perl              | 3 ++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-svn.txt b/Documentation/git-svn.txt
index 636e09048e..b858374649 100644
--- a/Documentation/git-svn.txt
+++ b/Documentation/git-svn.txt
@@ -657,6 +657,11 @@ config key: svn.authorsfile
 	expected to return a single line of the form "Name <email>",
 	which will be treated as if included in the authors file.
 +
+Due to historical reasons a relative 'filename' is first searched
+relative to the current directory for 'init' and 'clone' and relative
+to the root of the working tree for 'fetch'. If 'filename' is
+not found, it is searched like any other command in '$PATH'.
++
 [verse]
 config key: svn.authorsProg
 
diff --git a/git-svn.perl b/git-svn.perl
index a6b6c3e40c..050f2a36f4 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -374,7 +374,8 @@ version() if $_version;
 usage(1) unless defined $cmd;
 load_authors() if $_authors;
 if (defined $_authors_prog) {
-	$_authors_prog = "'" . File::Spec->rel2abs($_authors_prog) . "'";
+	my $abs_file = File::Spec->rel2abs($_authors_prog);
+	$_authors_prog = "'" . $abs_file . "'" if -x $abs_file;
 }
 
 unless ($cmd =~ /^(?:clone|init|multi-init|commit-diff)$/) {
-- 
2.16.2

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

[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 <asheiduk@gmail.com>
---
 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

Re: [PATCH 1/2] git-svn: search --authors-prog in PATH too

[Eric Sunshine]

On Sun, Mar 4, 2018 at 6:22 AM, Andreas Heiduk <asheiduk@gmail.com> wrote:
> In 36db1eddf9 ("git-svn: add --authors-prog option", 2009-05-14) the path
> to authors-prog was made absolute because git-svn changes the current
> directoy in some situations. This makes sense if the program is part of

s/directoy/directory/

> the repository but prevents searching via $PATH.
>
> The old behaviour is still retained, but if the file does not exists, then
> authors-prog is search in $PATH as any other command.

s/search/searched for/

> Signed-off-by: Andreas Heiduk <asheiduk@gmail.com>

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

[Eric Sunshine]

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?

Doesn't such a behavior change deserve being documented (and possibly tests)?

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

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

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

[Andreas Heiduk]

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?

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

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-avarab@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 1/2] git-svn: search --authors-prog in PATH too

[Eric Wong]

Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sun, Mar 4, 2018 at 6:22 AM, Andreas Heiduk <asheiduk@gmail.com> wrote:
> > In 36db1eddf9 ("git-svn: add --authors-prog option", 2009-05-14) the path
> > to authors-prog was made absolute because git-svn changes the current
> > directoy in some situations. This makes sense if the program is part of
> 
> s/directoy/directory/
> 
> > the repository but prevents searching via $PATH.
> >
> > The old behaviour is still retained, but if the file does not exists, then
> > authors-prog is search in $PATH as any other command.
> 
> s/search/searched for/
> 
> > Signed-off-by: Andreas Heiduk <asheiduk@gmail.com>

Thanks both, I've queued 1/2 up with Eric S's edits at svn/authors-prog.
I'm not yet convinced 2/2 is a good change, however.

The following changes since commit 7e31236f652ad9db221511eaf157ce0ef55585d6:

  Sixth batch for 2.17 (2018-02-28 13:39:24 -0800)

are available in the Git repository at:

  git://bogomips.org/git-svn.git svn/authors-prog

for you to fetch changes up to e99652c412701cf988770e5cfaa253712a39221a:

  git-svn: search --authors-prog in PATH too (2018-03-05 02:09:57 +0000)

----------------------------------------------------------------
Andreas Heiduk (1):
      git-svn: search --authors-prog in PATH too

 Documentation/git-svn.txt | 5 +++++
 git-svn.perl              | 3 ++-
 2 files changed, 7 insertions(+), 1 deletion(-)

Re: [PATCH 1/2] git-svn: search --authors-prog in PATH too

[Andreas Heiduk]

Am 05.03.2018 um 18:52 schrieb Eric Wong:
> 
> Thanks both, I've queued 1/2 up with Eric S's edits at svn/authors-prog.
> I'm not yet convinced 2/2 is a good change, however.

I'm not sure which direction your argument points to: Do you object to a
$PATH search at all? Or would you like to remove the legacy code too?
Or is the code/doc/... in bad shape?

Re: [PATCH 1/2] git-svn: search --authors-prog in PATH too

[Andreas Heiduk]

Am 05.03.2018 um 20:48 schrieb Andreas Heiduk:
> Am 05.03.2018 um 18:52 schrieb Eric Wong:
>> Thanks both, I've queued 1/2 up with Eric S's edits at svn/authors-prog.
>> I'm not yet convinced 2/2 is a good change, however.
> 
> I'm not sure which direction your argument points to: Do you object to a
> $PATH search at all? Or would you like to remove the legacy code too?
> Or is the code/doc/... in bad shape?

OK, I've mismatched your reply and the patch number. So my new questions
are:

Do you object to the feature as such? Or would you like to remove the
legacy code too? Or is the code/doc/... in bad shape beyond what E.S.
already mentioned?

Right now my team performs a hefty "git filter-branch" after each "git
svn fetch" followed by black magic to set the new object ids in the
caches / rev-maps of git-svn. Official support for "no synthetic email"
would be much easier.

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

[Eric Wong]

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

[Eric Sunshine]

On Mon, Mar 5, 2018 at 4:37 AM, Andreas Heiduk <asheiduk@gmail.com> wrote:
> 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.

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

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

[Andreas Heiduk]

Am 05.03.2018 um 10:37 schrieb Andreas Heiduk:
> 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:
>>> ---
>>> 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.