Fix merge parent checking with svn.pushmergeinfo.

PATCH In review Jason Merrill 2 participants 7 messages Sep 15, 2017 – Sep 16, 2017

Share & tools

Jason Merrill

author

2017-09-15 17:08 UTC #0 lore raw

PATCH 1/1 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://[email protected]/svn/gcc!"

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

Signed-off-by Jason Merrill <[email protected]>

---
 git-svn.perl | 1 +
 1 file changed, 1 insertion(+)

git-svn.perl +1 -0

index fa42364785..1663612b1c 100755
@@ -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);

Signature · 1 line

2.13.5

Jonathan Nieder

2017-09-15 17:52 UTC #1 lore raw

Hi,

Quoted 10 lines · Jason Merrill

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://[email protected]/svn/gcc!"
* git-svn.perl: Remove username from rooturl before comparing to branchurl.
Signed-off-by Jason Merrill <[email protected]>

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 sub cmd_dcommit { · line 934

Quoted 13 lines

---
 git-svn.perl | 1 +
 1 file changed, 1 insertion(+)

git-svn.perl +1 -0

index fa42364785..1663612b1c 100755
@@ -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);
		...

git-svn.perl sub cmd_dcommit { · line 937

Quoted 3 lines

git-svn.perl +0 -0

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

The rest looks good.

Thanks and hope that helps,
Jonathan

Jason Merrill

author

2017-09-15 21:28 UTC #2 lore raw

Quoted 11 lines · Jonathan Nieder

Hi,
Quoted 7 lines · Jason Merrill
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://[email protected]/svn/gcc!"

Quoted 5 lines

Quoted 3 lines
* git-svn.perl: Remove username from rooturl before comparing to branchurl.
Signed-off-by Jason Merrill <[email protected]>
Interesting. Thanks for writing it.

Thanks for the review.

Quoted 2 lines

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.

Quoted 9 lines

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://[email protected]/svn/gcc!"

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

Quoted 4 lines

---
 git-svn.perl | 1 +
 1 file changed, 1 insertion(+)

Quoted 12 lines

git-svn.perl sub cmd_dcommit { · line 934

Quoted 9 lines

git-svn.perl +1 -0

index fa42364785..1663612b1c 100755
@@ -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.

Quoted 10 lines

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

Quoted 3 lines

git-svn.perl +0 -0

              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.

Quoted 4 lines

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

Jason Merrill

author

2017-09-15 21:47 UTC #3 lore raw

PATCH v2 1/1 git-svn: Fix svn.pushmergeinfo handling of svn+ssh usernames.

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

Signed-off-by Jason Merrill <[email protected]>

Reviewed-by Jonathan Nieder <[email protected]>

---
 git-svn.perl | 1 +
 1 file changed, 1 insertion(+)

git-svn.perl +1 -0

index fa42364785..3b95d67bde 100755
@@ -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);

Signature · 1 line

2.13.5

Jonathan Nieder

Reviewed-by on v2 1/1

2017-09-15 21:54 UTC #4 lore raw

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

Quoted 5 lines · 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://[email protected]/svn/gcc!"

Quoted 8 lines

So, let's call remove_username (as we do for svn info) before comparing rooturl to branchurl.
Signed-off-by Jason Merrill <[email protected]>
Reviewed-by Jonathan Nieder <[email protected]>
---
 git-svn.perl | 1 +
 1 file changed, 1 insertion(+)

This is indeed

Reviewed-by Jonathan Nieder <[email protected]>

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

Jonathan Nieder

2017-09-15 21:53 UTC #5 lore raw

In reply to Jason Merrill

Quoted 16 lines · Jason Merrill

Quoted 9 lines · Jonathan Nieder
Quoted 7 lines · Jason Merrill
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://[email protected]/svn/gcc!"
Quoted 5 lines
Quoted 3 lines
* git-svn.perl: Remove username from rooturl before comparing to branchurl.
Signed-off-by Jason Merrill <[email protected]>
Interesting. Thanks for writing it.

Quoted 8 lines

Thanks for the review.
Quoted 2 lines
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?

[...]

Quoted 11 lines

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://[email protected]/svn/gcc!"
So, let's call remove_username (as we do for svn info) before comparing rooturl to branchurl.

Looks good.

Thanks.

Jonathan

Jason Merrill

author

2017-09-16 03:29 UTC #6 lore raw

Quoted 22 lines · Jonathan Nieder

Quoted 21 lines · Jason Merrill
Quoted 9 lines · Jonathan Nieder
Quoted 7 lines · Jason Merrill
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://[email protected]/svn/gcc!"
Quoted 5 lines
Quoted 3 lines
* git-svn.perl: Remove username from rooturl before comparing to branchurl.
Signed-off-by Jason Merrill <[email protected]>
Interesting. Thanks for writing it.
Thanks for the review.
Quoted 2 lines
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.

Quoted 8 lines

Quoted 3 lines
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!

Quoted 5 lines

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.

Quoted 2 lines

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