{"thread":{"id":"46759","subject":"[PATCH] Fix merge parent checking with svn.pushmergeinfo.","startedAt":"2017-09-15T17:08:35Z","lastAt":"2017-09-16T03:29:32Z","messageCount":7,"participants":["Jason Merrill","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"328135","messageId":"20170915170818.27390-1-jason@redhat.com","threadId":"46759","inReplyTo":null,"subject":"[PATCH] Fix merge parent checking with svn.pushmergeinfo.","fromName":"Jason Merrill","fromEmail":"jason@redhat.com","sentAt":"2017-09-15T17:08:18Z","receivedAt":"2017-09-15T17:08:35Z","isPatch":true,"sender":{"key":"jason@redhat.com","avatar":"https://avatars.githubusercontent.com/u/266146?v=4"},"body":"Without this fix, svn dcommit of a merge with svn.pushmergeinfo set would\nget error messages like \"merge parent for is on branch\nsvn+ssh://gcc.gnu.org/svn/gcc/trunk, which is not under the git-svn root\nsvn+ssh://jason@gcc.gnu.org/svn/gcc!\"\n\n* git-svn.perl: Remove username from rooturl before comparing to branchurl.\n\nSigned-off-by: Jason Merrill \n---\n git-svn.perl | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex fa42364785..1663612b1c 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -931,6 +931,7 @@ sub cmd_dcommit {\n \t\t# information from different SVN repos, and paths\n \t\t# which are not underneath this repository root.\n \t\tmy $rooturl = $gs->repos_root;\n+\t Git::SVN::remove_username ($rooturl);\n \t\tforeach my $d (@$linear_refs) {\n \t\t\tmy %parentshash;\n \t\t\tread_commit_parents(\\%parentshash, $d);\n-- \n2.13.5\n\n"},{"id":"328138","messageId":"20170915175248.GT27425@aiede.mtv.corp.google.com","threadId":"46759","inReplyTo":"20170915170818.27390-1-jason@redhat.com","subject":"Re: [PATCH] Fix merge parent checking with svn.pushmergeinfo.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-15T17:52:48Z","receivedAt":"2017-09-15T17:52:57Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJason Merrill wrote:\n\n> Subject: Fix merge parent checking with svn.pushmergeinfo.\n>\n> Without this fix, svn dcommit of a merge with svn.pushmergeinfo set would\n> get error messages like \"merge parent for is on branch\n> svn+ssh://gcc.gnu.org/svn/gcc/trunk, which is not under the git-svn root\n> svn+ssh://jason@gcc.gnu.org/svn/gcc!\"\n>\n> * git-svn.perl: Remove username from rooturl before comparing to branchurl.\n>\n> Signed-off-by: Jason Merrill \n\nInteresting. Thanks for writing it.\n\nCould there be a test for this to make sure this doesn't regress in\nthe future? See t/t9151-svn-mergeinfo.sh for some examples.\n\nNit: git doesn't use GNU-style changelogs, preferring to let the code\nspeak for itself. Maybe it would work better as the subject line?\nE.g. something like\n\n\tgit-svn: remove username from root before comparing to branch URL\n\n\tWithout this fix, ...\n\n\tSigned-off-by: ...\n\n> ---\n> git-svn.perl | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/git-svn.perl b/git-svn.perl\n> index fa42364785..1663612b1c 100755\n> --- a/git-svn.perl\n> +++ b/git-svn.perl\n> @@ -931,6 +931,7 @@ sub cmd_dcommit {\n> \t\t# information from different SVN repos, and paths\n> \t\t# which are not underneath this repository root.\n> \t\tmy $rooturl = $gs->repos_root;\n> +\t Git::SVN::remove_username ($rooturl);\n\nstyle nit: Git doesn't include a space between function names and\ntheir argument list.\n\nI wonder if it would make sense to rename the $rooturl variable\nsince now it is not the unmodified root. E.g. how about\n\n\t\tmy $expect_url = $gs->repos_root;\n\t\tGit::SVN::remove_username($expect_url);\n\t\t...\n\n> \t\tforeach my $d (@$linear_refs) {\n> \t\t\tmy %parentshash;\n> \t\t\tread_commit_parents(\\%parentshash, $d);\n\nThe rest looks good.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"328171","messageId":"CADzB+2mxyXcROYx72tac8cUxBMxi=ZxUNQYxpUR1CZ43e-j9gA@mail.gmail.com","threadId":"46759","inReplyTo":"20170915175248.GT27425@aiede.mtv.corp.google.com","subject":"Re: [PATCH] Fix merge parent checking with svn.pushmergeinfo.","fromName":"Jason Merrill","fromEmail":"jason@redhat.com","sentAt":"2017-09-15T21:28:12Z","receivedAt":"2017-09-15T21:28:38Z","isPatch":true,"sender":{"key":"jason@redhat.com","avatar":"https://avatars.githubusercontent.com/u/266146?v=4"},"body":"On Fri, Sep 15, 2017 at 1:52 PM, Jonathan Nieder wrote:\n> Hi,\n>\n> Jason Merrill wrote:\n>\n>> Subject: Fix merge parent checking with svn.pushmergeinfo.\n>>\n>> Without this fix, svn dcommit of a merge with svn.pushmergeinfo set would\n>> get error messages like \"merge parent for is on branch\n>> svn+ssh://gcc.gnu.org/svn/gcc/trunk, which is not under the git-svn root\n>> svn+ssh://jason@gcc.gnu.org/svn/gcc!\"\n>>\n>> * git-svn.perl: Remove username from rooturl before comparing to branchurl.\n>>\n>> Signed-off-by: Jason Merrill \n>\n> Interesting. Thanks for writing it.\n\nThanks for the review.\n\n> Could there be a test for this to make sure this doesn't regress in\n> the future? See t/t9151-svn-mergeinfo.sh for some examples.\n\nHmm, I'm afraid figuring out how to write such a test would take\nlonger than I can really spare for this issue. There don't seem to be\nany svn+ssh tests currently.\n\n> Nit: git doesn't use GNU-style changelogs, preferring to let the code\n> speak for itself. Maybe it would work better as the subject line?\n> E.g. something like\n>\n> git-svn: remove username from root before comparing to branch URL\n>\n> Without this fix, ...\n>\n> Signed-off-by: ...\n\nHow about this?\n\n git-svn: Fix svn.pushmergeinfo handling of svn+ssh usernames.\n\n Previously, svn dcommit of a merge with svn.pushmergeinfo set would\n get error messages like \"merge parent for is on branch\n svn+ssh://gcc.gnu.org/svn/gcc/trunk, which is not under the git-svn root\n svn+ssh://jason@gcc.gnu.org/svn/gcc!\"\n\n So, let's call remove_username (as we do for svn info) before comparing\n rooturl to branchurl.\n\n>> ---\n>> git-svn.perl | 1 +\n>> 1 file changed, 1 insertion(+)\n>>\n>> diff --git a/git-svn.perl b/git-svn.perl\n>> index fa42364785..1663612b1c 100755\n>> --- a/git-svn.perl\n>> +++ b/git-svn.perl\n>> @@ -931,6 +931,7 @@ sub cmd_dcommit {\n>> # information from different SVN repos, and paths\n>> # which are not underneath this repository root.\n>> my $rooturl = $gs->repos_root;\n>> + Git::SVN::remove_username ($rooturl);\n>\n> style nit: Git doesn't include a space between function names and\n> their argument list.\n\nFixed.\n\n> I wonder if it would make sense to rename the $rooturl variable\n> since now it is not the unmodified root. E.g. how about\n>\n> my $expect_url = $gs->repos_root;\n> Git::SVN::remove_username($expect_url);\n> ...\n>\n>> foreach my $d (@$linear_refs) {\n>> my %parentshash;\n>> read_commit_parents(\\%parentshash, $d);\n\nIt isn't the unmodified root, but it is the effective root that is\nprinted by svn info and used in branch URLs in git-svn-id, so it seems\nto me that the name $rooturl is still appropriate.\n\n> The rest looks good.\n>\n> Thanks and hope that helps,\n> Jonathan\n"},{"id":"328174","messageId":"20170915214653.14720-1-jason@redhat.com","threadId":"46759","inReplyTo":"CADzB+2mxyXcROYx72tac8cUxBMxi=ZxUNQYxpUR1CZ43e-j9gA@mail.gmail.com","subject":"[PATCH v2] git-svn: Fix svn.pushmergeinfo handling of svn+ssh usernames.","fromName":"Jason Merrill","fromEmail":"jason@redhat.com","sentAt":"2017-09-15T21:46:53Z","receivedAt":"2017-09-15T21:47:30Z","isPatch":true,"sender":{"key":"jason@redhat.com","avatar":"https://avatars.githubusercontent.com/u/266146?v=4"},"body":"Previously, svn dcommit of a merge with svn.pushmergeinfo set would\nget error messages like \"merge parent for is on branch\nsvn+ssh://gcc.gnu.org/svn/gcc/trunk, which is not under the git-svn root\nsvn+ssh://jason@gcc.gnu.org/svn/gcc!\"\n\nSo, let's call remove_username (as we do for svn info) before comparing\nrooturl to branchurl.\n\nSigned-off-by: Jason Merrill \nReviewed-by: Jonathan Nieder \n---\n git-svn.perl | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex fa42364785..3b95d67bde 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -931,6 +931,7 @@ sub cmd_dcommit {\n \t\t# information from different SVN repos, and paths\n \t\t# which are not underneath this repository root.\n \t\tmy $rooturl = $gs->repos_root;\n+\t Git::SVN::remove_username($rooturl);\n \t\tforeach my $d (@$linear_refs) {\n \t\t\tmy %parentshash;\n \t\t\tread_commit_parents(\\%parentshash, $d);\n-- \n2.13.5\n\n"},{"id":"328175","messageId":"20170915215303.GV27425@aiede.mtv.corp.google.com","threadId":"46759","inReplyTo":"CADzB+2mxyXcROYx72tac8cUxBMxi=ZxUNQYxpUR1CZ43e-j9gA@mail.gmail.com","subject":"Re: [PATCH] Fix merge parent checking with svn.pushmergeinfo.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-15T21:53:03Z","receivedAt":"2017-09-15T21:53:12Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jason Merrill wrote:\n> On Fri, Sep 15, 2017 at 1:52 PM, Jonathan Nieder wrote:\n> > Jason Merrill wrote:\n\n>>> Subject: Fix merge parent checking with svn.pushmergeinfo.\n>>>\n>>> Without this fix, svn dcommit of a merge with svn.pushmergeinfo set would\n>>> get error messages like \"merge parent for is on branch\n>>> svn+ssh://gcc.gnu.org/svn/gcc/trunk, which is not under the git-svn root\n>>> svn+ssh://jason@gcc.gnu.org/svn/gcc!\"\n>>>\n>>> * git-svn.perl: Remove username from rooturl before comparing to branchurl.\n>>>\n>>> Signed-off-by: Jason Merrill \n>>\n>> Interesting. Thanks for writing it.\n>\n> Thanks for the review.\n>\n>> Could there be a test for this to make sure this doesn't regress in\n>> the future? See t/t9151-svn-mergeinfo.sh for some examples.\n>\n> Hmm, I'm afraid figuring out how to write such a test would take\n> longer than I can really spare for this issue. There don't seem to be\n> any svn+ssh tests currently.\n\nWell, could you give manual commands to allow me to reproduce the\nproblem?\n\nThen I'll translate them into a test. :)\n\nFWIW remove_username seems to be able to cope fine with an http://\nURL. t/lib-httpd.sh starts an http server with Subversion enabled,\nas long as the envvar GIT_SVN_TEST_HTTPD is set to true. Its address\nis $svnrepo, which is an http URL (but I don't see a username in the\nURL). Does that help?\n\nAlternatively, does using rewrite-root as in t9151-svn-mergeinfo.sh\nhelp?\n\n[...]\n> How about this?\n>\n> git-svn: Fix svn.pushmergeinfo handling of svn+ssh usernames.\n>\n> Previously, svn dcommit of a merge with svn.pushmergeinfo set would\n> get error messages like \"merge parent for is on branch\n> svn+ssh://gcc.gnu.org/svn/gcc/trunk, which is not under the git-svn root\n> svn+ssh://jason@gcc.gnu.org/svn/gcc!\"\n>\n> So, let's call remove_username (as we do for svn info) before comparing\n> rooturl to branchurl.\n\nLooks good.\n\nThanks.\n\nJonathan\n"},{"id":"328176","messageId":"20170915215413.GW27425@aiede.mtv.corp.google.com","threadId":"46759","inReplyTo":"20170915214653.14720-1-jason@redhat.com","subject":"Re: [PATCH v2] git-svn: Fix svn.pushmergeinfo handling of svn+ssh usernames.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-15T21:54:13Z","receivedAt":"2017-09-15T21:54:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jason Merrill wrote:\n\n> Previously, svn dcommit of a merge with svn.pushmergeinfo set would\n> get error messages like \"merge parent for is on branch\n> svn+ssh://gcc.gnu.org/svn/gcc/trunk, which is not under the git-svn root\n> svn+ssh://jason@gcc.gnu.org/svn/gcc!\"\n>\n> So, let's call remove_username (as we do for svn info) before comparing\n> rooturl to branchurl.\n>\n> Signed-off-by: Jason Merrill \n> Reviewed-by: Jonathan Nieder \n> ---\n> git-svn.perl | 1 +\n> 1 file changed, 1 insertion(+)\n\nThis is indeed\nReviewed-by: Jonathan Nieder \n\nthough it does need a test --- I have no confidence that this fix will\nbe preserved without one. Anyway, that can happen in a separate\npatch.\n\nThanks for your work,\nJonathan\n"},{"id":"328179","messageId":"CADzB+2mT=Ht5AQybp+44+PMbRrWXB4dPyTmsEgfVkOOujZYQxQ@mail.gmail.com","threadId":"46759","inReplyTo":"20170915215303.GV27425@aiede.mtv.corp.google.com","subject":"Re: [PATCH] Fix merge parent checking with svn.pushmergeinfo.","fromName":"Jason Merrill","fromEmail":"jason@redhat.com","sentAt":"2017-09-16T03:29:06Z","receivedAt":"2017-09-16T03:29:32Z","isPatch":true,"sender":{"key":"jason@redhat.com","avatar":"https://avatars.githubusercontent.com/u/266146?v=4"},"body":"On Fri, Sep 15, 2017 at 5:53 PM, Jonathan Nieder wrote:\n> Jason Merrill wrote:\n>> On Fri, Sep 15, 2017 at 1:52 PM, Jonathan Nieder wrote:\n>> > Jason Merrill wrote:\n>\n>>>> Subject: Fix merge parent checking with svn.pushmergeinfo.\n>>>>\n>>>> Without this fix, svn dcommit of a merge with svn.pushmergeinfo set would\n>>>> get error messages like \"merge parent for is on branch\n>>>> svn+ssh://gcc.gnu.org/svn/gcc/trunk, which is not under the git-svn root\n>>>> svn+ssh://jason@gcc.gnu.org/svn/gcc!\"\n>>>>\n>>>> * git-svn.perl: Remove username from rooturl before comparing to branchurl.\n>>>>\n>>>> Signed-off-by: Jason Merrill \n>>>\n>>> Interesting. Thanks for writing it.\n>>\n>> Thanks for the review.\n>>\n>>> Could there be a test for this to make sure this doesn't regress in\n>>> the future? See t/t9151-svn-mergeinfo.sh for some examples.\n>>\n>> Hmm, I'm afraid figuring out how to write such a test would take\n>> longer than I can really spare for this issue. There don't seem to be\n>> any svn+ssh tests currently.\n>\n> Well, could you give manual commands to allow me to reproduce the\n> problem?\n>\n> Then I'll translate them into a test. :)\n\nSomething like this:\n\ngit svn clone -s svn+ssh://user@host/repo\ngit config svn.pushmergeinfo yes\ngit checkout -b branch origin/branch\ngit merge origin/trunk\ngit svn dcommit\n\nThanks!\n\n> FWIW remove_username seems to be able to cope fine with an http://\n> URL. t/lib-httpd.sh starts an http server with Subversion enabled,\n> as long as the envvar GIT_SVN_TEST_HTTPD is set to true. Its address\n> is $svnrepo, which is an http URL (but I don't see a username in the\n> URL). Does that help?\n\nI think the http transport handles the username separately, not in the URL.\n\nI would expect that a dummy ssh wrapper like some of the tests use\nwould be sufficient, no need for an actual network connection.\n\n> Alternatively, does using rewrite-root as in t9151-svn-mergeinfo.sh\n> help?\n\nHmm, I'm not sure how rewriteRoot would interact with this issue,\nwhether it would be useful as a workaround or another problematic\ncase.\n\nJason\n"}]}