[PATCH] Fix merge parent checking with svn.pushmergeinfo.

[PATCH] Fix merge parent checking with svn.pushmergeinfo.

[Jason Merrill]

Without this fix, svn dcommit of a merge with svn.pushmergeinfo set would
get error messages like "merge parent <X> for <Y> is on branch
svn+ssh://gcc.gnu.org/svn/gcc/trunk, which is not under the git-svn root
svn+ssh://jason@gcc.gnu.org/svn/gcc!"

* git-svn.perl: Remove username from rooturl before comparing to branchurl.

Signed-off-by: Jason Merrill <jason@redhat.com>
---
 git-svn.perl | 1 +
 1 file changed, 1 insertion(+)

diff --git a/git-svn.perl b/git-svn.perl
index fa42364785..1663612b1c 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -931,6 +931,7 @@ sub cmd_dcommit {
 		# information from different SVN repos, and paths
 		# which are not underneath this repository root.
 		my $rooturl = $gs->repos_root;
+	        Git::SVN::remove_username ($rooturl);
 		foreach my $d (@$linear_refs) {
 			my %parentshash;
 			read_commit_parents(\%parentshash, $d);
-- 
2.13.5

Re: [PATCH] Fix merge parent checking with svn.pushmergeinfo.

[Jonathan Nieder]

Hi,

Jason Merrill wrote:

> Subject: Fix merge parent checking with svn.pushmergeinfo.
>
> Without this fix, svn dcommit of a merge with svn.pushmergeinfo set would
> get error messages like "merge parent <X> for <Y> is on branch
> svn+ssh://gcc.gnu.org/svn/gcc/trunk, which is not under the git-svn root
> svn+ssh://jason@gcc.gnu.org/svn/gcc!"
>
> * git-svn.perl: Remove username from rooturl before comparing to branchurl.
>
> Signed-off-by: Jason Merrill <jason@redhat.com>

Interesting.  Thanks for writing it.

Could there be a test for this to make sure this doesn't regress in
the future?  See t/t9151-svn-mergeinfo.sh for some examples.

Nit: git doesn't use GNU-style changelogs, preferring to let the code
speak for itself.  Maybe it would work better as the subject line?
E.g. something like

	git-svn: remove username from root before comparing to branch URL

	Without this fix, ...

	Signed-off-by: ...

> ---
>  git-svn.perl | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/git-svn.perl b/git-svn.perl
> index fa42364785..1663612b1c 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -931,6 +931,7 @@ sub cmd_dcommit {
>  		# information from different SVN repos, and paths
>  		# which are not underneath this repository root.
>  		my $rooturl = $gs->repos_root;
> +	        Git::SVN::remove_username ($rooturl);

style nit: Git doesn't include a space between function names and
their argument list.

I wonder if it would make sense to rename the $rooturl variable
since now it is not the unmodified root. E.g. how about

		my $expect_url = $gs->repos_root;
		Git::SVN::remove_username($expect_url);
		...

>  		foreach my $d (@$linear_refs) {
>  			my %parentshash;
>  			read_commit_parents(\%parentshash, $d);

The rest looks good.

Thanks and hope that helps,
Jonathan

Re: [PATCH] Fix merge parent checking with svn.pushmergeinfo.

[Jason Merrill]

On Fri, Sep 15, 2017 at 1:52 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Hi,
>
> Jason Merrill wrote:
>
>> Subject: Fix merge parent checking with svn.pushmergeinfo.
>>
>> Without this fix, svn dcommit of a merge with svn.pushmergeinfo set would
>> get error messages like "merge parent <X> for <Y> is on branch
>> svn+ssh://gcc.gnu.org/svn/gcc/trunk, which is not under the git-svn root
>> svn+ssh://jason@gcc.gnu.org/svn/gcc!"
>>
>> * git-svn.perl: Remove username from rooturl before comparing to branchurl.
>>
>> Signed-off-by: Jason Merrill <jason@redhat.com>
>
> Interesting.  Thanks for writing it.

Thanks for the review.

> Could there be a test for this to make sure this doesn't regress in
> the future?  See t/t9151-svn-mergeinfo.sh for some examples.

Hmm, I'm afraid figuring out how to write such a test would take
longer than I can really spare for this issue.  There don't seem to be
any svn+ssh tests currently.

> Nit: git doesn't use GNU-style changelogs, preferring to let the code
> speak for itself.  Maybe it would work better as the subject line?
> E.g. something like
>
>         git-svn: remove username from root before comparing to branch URL
>
>         Without this fix, ...
>
>         Signed-off-by: ...

How about this?

    git-svn: Fix svn.pushmergeinfo handling of svn+ssh usernames.

    Previously, svn dcommit of a merge with svn.pushmergeinfo set would
    get error messages like "merge parent <X> for <Y> is on branch
    svn+ssh://gcc.gnu.org/svn/gcc/trunk, which is not under the git-svn root
    svn+ssh://jason@gcc.gnu.org/svn/gcc!"

    So, let's call remove_username (as we do for svn info) before comparing
    rooturl to branchurl.

>> ---
>>  git-svn.perl | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/git-svn.perl b/git-svn.perl
>> index fa42364785..1663612b1c 100755
>> --- a/git-svn.perl
>> +++ b/git-svn.perl
>> @@ -931,6 +931,7 @@ sub cmd_dcommit {
>>               # information from different SVN repos, and paths
>>               # which are not underneath this repository root.
>>               my $rooturl = $gs->repos_root;
>> +             Git::SVN::remove_username ($rooturl);
>
> style nit: Git doesn't include a space between function names and
> their argument list.

Fixed.

> I wonder if it would make sense to rename the $rooturl variable
> since now it is not the unmodified root. E.g. how about
>
>                 my $expect_url = $gs->repos_root;
>                 Git::SVN::remove_username($expect_url);
>                 ...
>
>>               foreach my $d (@$linear_refs) {
>>                       my %parentshash;
>>                       read_commit_parents(\%parentshash, $d);

It isn't the unmodified root, but it is the effective root that is
printed by svn info and used in branch URLs in git-svn-id, so it seems
to me that the name $rooturl is still appropriate.

> The rest looks good.
>
> Thanks and hope that helps,
> Jonathan

[PATCH v2] git-svn: Fix svn.pushmergeinfo handling of svn+ssh usernames.

[Jason Merrill]

Previously, svn dcommit of a merge with svn.pushmergeinfo set would
get error messages like "merge parent <X> for <Y> is on branch
svn+ssh://gcc.gnu.org/svn/gcc/trunk, which is not under the git-svn root
svn+ssh://jason@gcc.gnu.org/svn/gcc!"

So, let's call remove_username (as we do for svn info) before comparing
rooturl to branchurl.

Signed-off-by: Jason Merrill <jason@redhat.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---
 git-svn.perl | 1 +
 1 file changed, 1 insertion(+)

diff --git a/git-svn.perl b/git-svn.perl
index fa42364785..3b95d67bde 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -931,6 +931,7 @@ sub cmd_dcommit {
 		# information from different SVN repos, and paths
 		# which are not underneath this repository root.
 		my $rooturl = $gs->repos_root;
+	        Git::SVN::remove_username($rooturl);
 		foreach my $d (@$linear_refs) {
 			my %parentshash;
 			read_commit_parents(\%parentshash, $d);
-- 
2.13.5

Re: [PATCH] Fix merge parent checking with svn.pushmergeinfo.

[Jonathan Nieder]

Jason Merrill wrote:
> On Fri, Sep 15, 2017 at 1:52 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> > Jason Merrill wrote:

>>> Subject: Fix merge parent checking with svn.pushmergeinfo.
>>>
>>> Without this fix, svn dcommit of a merge with svn.pushmergeinfo set would
>>> get error messages like "merge parent <X> for <Y> is on branch
>>> svn+ssh://gcc.gnu.org/svn/gcc/trunk, which is not under the git-svn root
>>> svn+ssh://jason@gcc.gnu.org/svn/gcc!"
>>>
>>> * git-svn.perl: Remove username from rooturl before comparing to branchurl.
>>>
>>> Signed-off-by: Jason Merrill <jason@redhat.com>
>>
>> Interesting.  Thanks for writing it.
>
> Thanks for the review.
>
>> Could there be a test for this to make sure this doesn't regress in
>> the future?  See t/t9151-svn-mergeinfo.sh for some examples.
>
> Hmm, I'm afraid figuring out how to write such a test would take
> longer than I can really spare for this issue.  There don't seem to be
> any svn+ssh tests currently.

Well, could you give manual commands to allow me to reproduce the
problem?

Then I'll translate them into a test. :)

FWIW remove_username seems to be able to cope fine with an http://
URL.  t/lib-httpd.sh starts an http server with Subversion enabled,
as long as the envvar GIT_SVN_TEST_HTTPD is set to true.  Its address
is $svnrepo, which is an http URL (but I don't see a username in the
URL).  Does that help?

Alternatively, does using rewrite-root as in t9151-svn-mergeinfo.sh
help?

[...]
> How about this?
>
>     git-svn: Fix svn.pushmergeinfo handling of svn+ssh usernames.
>
>     Previously, svn dcommit of a merge with svn.pushmergeinfo set would
>     get error messages like "merge parent <X> for <Y> is on branch
>     svn+ssh://gcc.gnu.org/svn/gcc/trunk, which is not under the git-svn root
>     svn+ssh://jason@gcc.gnu.org/svn/gcc!"
>
>     So, let's call remove_username (as we do for svn info) before comparing
>     rooturl to branchurl.

Looks good.

Thanks.

Jonathan

Re: [PATCH v2] git-svn: Fix svn.pushmergeinfo handling of svn+ssh usernames.

[Jonathan Nieder]

Jason Merrill wrote:

> Previously, svn dcommit of a merge with svn.pushmergeinfo set would
> get error messages like "merge parent <X> for <Y> is on branch
> svn+ssh://gcc.gnu.org/svn/gcc/trunk, which is not under the git-svn root
> svn+ssh://jason@gcc.gnu.org/svn/gcc!"
>
> So, let's call remove_username (as we do for svn info) before comparing
> rooturl to branchurl.
>
> Signed-off-by: Jason Merrill <jason@redhat.com>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
>  git-svn.perl | 1 +
>  1 file changed, 1 insertion(+)

This is indeed
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

though it does need a test --- I have no confidence that this fix will
be preserved without one.  Anyway, that can happen in a separate
patch.

Thanks for your work,
Jonathan

Re: [PATCH] Fix merge parent checking with svn.pushmergeinfo.

[Jason Merrill]

On Fri, Sep 15, 2017 at 5:53 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Jason Merrill wrote:
>> On Fri, Sep 15, 2017 at 1:52 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> > Jason Merrill wrote:
>
>>>> Subject: Fix merge parent checking with svn.pushmergeinfo.
>>>>
>>>> Without this fix, svn dcommit of a merge with svn.pushmergeinfo set would
>>>> get error messages like "merge parent <X> for <Y> is on branch
>>>> svn+ssh://gcc.gnu.org/svn/gcc/trunk, which is not under the git-svn root
>>>> svn+ssh://jason@gcc.gnu.org/svn/gcc!"
>>>>
>>>> * git-svn.perl: Remove username from rooturl before comparing to branchurl.
>>>>
>>>> Signed-off-by: Jason Merrill <jason@redhat.com>
>>>
>>> Interesting.  Thanks for writing it.
>>
>> Thanks for the review.
>>
>>> Could there be a test for this to make sure this doesn't regress in
>>> the future?  See t/t9151-svn-mergeinfo.sh for some examples.
>>
>> Hmm, I'm afraid figuring out how to write such a test would take
>> longer than I can really spare for this issue.  There don't seem to be
>> any svn+ssh tests currently.
>
> Well, could you give manual commands to allow me to reproduce the
> problem?
>
> Then I'll translate them into a test. :)

Something like this:

git svn clone -s svn+ssh://user@host/repo
git config svn.pushmergeinfo yes
git checkout -b branch origin/branch
git merge origin/trunk
git svn dcommit

Thanks!

> FWIW remove_username seems to be able to cope fine with an http://
> URL.  t/lib-httpd.sh starts an http server with Subversion enabled,
> as long as the envvar GIT_SVN_TEST_HTTPD is set to true.  Its address
> is $svnrepo, which is an http URL (but I don't see a username in the
> URL).  Does that help?

I think the http transport handles the username separately, not in the URL.

I would expect that a dummy ssh wrapper like some of the tests use
would be sufficient, no need for an actual network connection.

> Alternatively, does using rewrite-root as in t9151-svn-mergeinfo.sh
> help?

Hmm, I'm not sure how rewriteRoot would interact with this issue,
whether it would be useful as a workaround or another problematic
case.

Jason