{"thread":{"id":"36263","subject":"[PATCH v3] remote-hg: do not fail on invalid bookmarks","startedAt":"2014-03-21T11:36:36Z","lastAt":"2014-03-22T16:41:37Z","messageCount":5,"participants":["Max Horn","Torsten Bögershausen","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"237308","messageId":"A4F451CA-D1DE-43A9-A4DA-23594C08C4DD@quendi.de","threadId":"36263","inReplyTo":null,"subject":"[PATCH v3] remote-hg: do not fail on invalid bookmarks","fromName":"Max Horn","fromEmail":"max@quendi.de","sentAt":"2014-03-21T11:36:36Z","receivedAt":"2014-03-21T11:36:36Z","isPatch":true,"sender":{"key":"max@quendi.de","avatar":"https://avatars.githubusercontent.com/u/241512?v=4"},"body":"Mercurial can have bookmarks pointing to \"nullid\" (the empty root\nrevision), while Git can not have references to it. When cloning or\nfetching from a Mercurial repository that has such a bookmark, the\nimport failed because git-remote-hg was not be able to create the\ncorresponding reference.\n\nWarn the user about the invalid reference, and do not advertise these\nbookmarks as head refs, but otherwise continue the import. In\nparticular, we still keep track of the fact that the remote repository\nhas a bookmark of the given name, in case the user wants to modify that\nbookmark.\n\nAlso add some test cases for this issue.\n\nReported-by: Antoine Pelisse <apelisse@gmail.com>\nSigned-off-by: Max Horn <max@quendi.de>\n---\nThis is a different fix than in my previous attempts. I thought\na bit more about the issue, and determined that the previous fix,\nwhile working, was not really correct: It is wrong to\ntreat nullid bookmarks as if they are non-existent; if e.g.\nthe user wants to modify the bookmark from git, we need to\ninto account that the remote already has a bookmark with that name.\nIndeed, I extended the new test cases to cover this aspect.\nWith the previous fix, the new tests would fail upon pushing,\nwith the new one, they work.\n\n contrib/remote-helpers/git-remote-hg |  5 ++-\n contrib/remote-helpers/test-hg.sh    | 67 ++++++++++++++++++++++++++++++++++++\n 2 files changed, 71 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg\nindex eb89ef6..36b5261 100755\n--- a/contrib/remote-helpers/git-remote-hg\n+++ b/contrib/remote-helpers/git-remote-hg\n@@ -643,7 +643,10 @@ def do_list(parser):\n             print \"? refs/heads/branches/%s\" % gitref(branch)\n \n     for bmark in bmarks:\n-        print \"? refs/heads/%s\" % gitref(bmark)\n+        if  bmarks[bmark].hex() == '0000000000000000000000000000000000000000':\n+            warn(\"Ignoring invalid bookmark '%s'\", bmark)\n+        else:\n+            print \"? refs/heads/%s\" % gitref(bmark)\n \n     for tag, node in repo.tagslist():\n         if tag == 'tip':\ndiff --git a/contrib/remote-helpers/test-hg.sh b/contrib/remote-helpers/test-hg.sh\nindex a933b1e..f5d0d97 100755\n--- a/contrib/remote-helpers/test-hg.sh\n+++ b/contrib/remote-helpers/test-hg.sh\n@@ -772,4 +772,71 @@ test_expect_success 'remote double failed push' '\n \t)\n '\n \n+test_expect_success 'clone remote with master null bookmark, then push to the bookmark' '\n+\ttest_when_finished \"rm -rf gitrepo* hgrepo*\" &&\n+\n+\t(\n+\thg init hgrepo &&\n+\tcd hgrepo &&\n+\techo a >a &&\n+\thg add a &&\n+\thg commit -m a &&\n+\thg bookmark -r null master\n+\t) &&\n+\n+\tgit clone \"hg::hgrepo\" gitrepo &&\n+\tcheck gitrepo HEAD a &&\n+\tcd gitrepo &&\n+\tgit checkout --quiet -b master &&\n+\techo b >b &&\n+\tgit add b &&\n+\tgit commit -m b &&\n+\tgit push origin master\n+'\n+\n+test_expect_success 'clone remote with default null bookmark, then push to the bookmark' '\n+\ttest_when_finished \"rm -rf gitrepo* hgrepo*\" &&\n+\n+\t(\n+\thg init hgrepo &&\n+\tcd hgrepo &&\n+\techo a >a &&\n+\thg add a &&\n+\thg commit -m a &&\n+\thg bookmark -r null -f default\n+\t) &&\n+\n+\tgit clone \"hg::hgrepo\" gitrepo &&\n+\tcheck gitrepo HEAD a &&\n+\tcd gitrepo &&\n+\tgit checkout --quiet -b default &&\n+\techo b >b &&\n+\tgit add b &&\n+\tgit commit -m b &&\n+\tgit push origin default\n+'\n+\n+test_expect_success 'clone remote with generic null bookmark, then push to the bookmark' '\n+\ttest_when_finished \"rm -rf gitrepo* hgrepo*\" &&\n+\n+\t(\n+\thg init hgrepo &&\n+\tcd hgrepo &&\n+\techo a >a &&\n+\thg add a &&\n+\thg commit -m a &&\n+\thg bookmark -r null bmark\n+\t) &&\n+\n+\tgit clone \"hg::hgrepo\" gitrepo &&\n+\tcheck gitrepo HEAD a &&\n+\tcd gitrepo &&\n+\tgit checkout --quiet -b bmark &&\n+\tgit remote -v &&\n+\techo b >b &&\n+\tgit add b &&\n+\tgit commit -m b &&\n+\tgit push origin bmark\n+'\n+\n test_done\n-- \n1.9.0.7.ga299b13\n"},{"id":"237359","messageId":"532CA557.20007@web.de","threadId":"36263","inReplyTo":"A4F451CA-D1DE-43A9-A4DA-23594C08C4DD@quendi.de","subject":"Re: [PATCH v3] remote-hg: do not fail on invalid bookmarks","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-03-21T20:47:19Z","receivedAt":"2014-03-21T20:47:19Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2014-03-21 12.36, Max Horn wrote:\nAll tests passed :-), thanks from my side.\ncomments inline, some are debatable\n> Mercurial can have bookmarks pointing to \"nullid\" (the empty root\n> revision), while Git can not have references to it. When cloning or\n> fetching from a Mercurial repository that has such a bookmark, the\n> import failed because git-remote-hg was not be able to create the\n> corresponding reference.\n>\n> Warn the user about the invalid reference, and do not advertise these\n> bookmarks as head refs, but otherwise continue the import. In\n> particular, we still keep track of the fact that the remote repository\n> has a bookmark of the given name, in case the user wants to modify that\n> bookmark.\n>\n> Also add some test cases for this issue.\ns/some test cases/test cases/\n>\n> Reported-by: Antoine Pelisse <apelisse@gmail.com>\n> Signed-off-by: Max Horn <max@quendi.de>\n> ---\n> This is a different fix than in my previous attempts. I thought\n> a bit more about the issue, and determined that the previous fix,\n> while working, was not really correct: It is wrong to\n> treat nullid bookmarks as if they are non-existent; if e.g.\n> the user wants to modify the bookmark from git, we need to\n> into account that the remote already has a bookmark with that name.\n> Indeed, I extended the new test cases to cover this aspect.\n> With the previous fix, the new tests would fail upon pushing,\n> with the new one, they work.\n>\n>  contrib/remote-helpers/git-remote-hg |  5 ++-\n>  contrib/remote-helpers/test-hg.sh    | 67 ++++++++++++++++++++++++++++++++++++\n>  2 files changed, 71 insertions(+), 1 deletion(-)\n>\n> diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg\n> index eb89ef6..36b5261 100755\n> --- a/contrib/remote-helpers/git-remote-hg\n> +++ b/contrib/remote-helpers/git-remote-hg\n> @@ -643,7 +643,10 @@ def do_list(parser):\n>              print \"? refs/heads/branches/%s\" % gitref(branch)\n>  \n>      for bmark in bmarks:\n> -        print \"? refs/heads/%s\" % gitref(bmark)\n> +        if  bmarks[bmark].hex() == '0000000000000000000000000000000000000000':\n> +            warn(\"Ignoring invalid bookmark '%s'\", bmark)\n> +        else:\n> +            print \"? refs/heads/%s\" % gitref(bmark)\n>  \n>      for tag, node in repo.tagslist():\n>          if tag == 'tip':\n> diff --git a/contrib/remote-helpers/test-hg.sh b/contrib/remote-helpers/test-hg.sh\n> index a933b1e..f5d0d97 100755\n> --- a/contrib/remote-helpers/test-hg.sh\n> +++ b/contrib/remote-helpers/test-hg.sh\n> @@ -772,4 +772,71 @@ test_expect_success 'remote double failed push' '\n>  \t)\n>  '\n>  \n> +test_expect_success 'clone remote with master null bookmark, then push to the bookmark' '\n> +\ttest_when_finished \"rm -rf gitrepo* hgrepo*\" &&\n> +\n> +\t(\n> +\thg init hgrepo &&\n> +\tcd hgrepo &&\nMinor:\nWe can change the order here, to make the \"cd hgrepo\" the first line in the subshell:\n\n+\thg init hgrepo &&\n+\t(\n+\tcd hgrepo &&\n\n\n> +\techo a >a &&\n> +\thg add a &&\n> +\thg commit -m a &&\n> +\thg bookmark -r null master\n> +\t) &&\n> +\n> +\tgit clone \"hg::hgrepo\" gitrepo &&\n> +\tcheck gitrepo HEAD a &&\nAnd here we do \"cd\", and this should be done in a subshell\n> +\tcd gitrepo &&\n> +\tgit checkout --quiet -b master &&\n> +\techo b >b &&\n> +\tgit add b &&\n> +\tgit commit -m b &&\n> +\tgit push origin master\n> +'\n> +\n> +test_expect_success 'clone remote with default null bookmark, then push to the bookmark' '\n> +\ttest_when_finished \"rm -rf gitrepo* hgrepo*\" &&\n> +\n> +\t(\n> +\thg init hgrepo &&\n> +\tcd hgrepo &&\n(Same minor as above)\n> +\techo a >a &&\n> +\thg add a &&\n> +\thg commit -m a &&\n> +\thg bookmark -r null -f default\n> +\t) &&\n> +\n> +\tgit clone \"hg::hgrepo\" gitrepo &&\n> +\tcheck gitrepo HEAD a &&\n> +\tcd gitrepo &&\n> +\tgit checkout --quiet -b default &&\n> +\techo b >b &&\n> +\tgit add b &&\n> +\tgit commit -m b &&\n> +\tgit push origin default\n> +'\n> +\n> +test_expect_success 'clone remote with generic null bookmark, then push to the bookmark' '\n> +\ttest_when_finished \"rm -rf gitrepo* hgrepo*\" &&\n> +\n> +\t(\n> +\thg init hgrepo &&\n> +\tcd hgrepo &&\n(Same as above)\n> +\techo a >a &&\n> +\thg add a &&\n> +\thg commit -m a &&\n> +\thg bookmark -r null bmark\n> +\t) &&\n> +\n> +\tgit clone \"hg::hgrepo\" gitrepo &&\n> +\tcheck gitrepo HEAD a &&\n> +\tcd gitrepo &&\nSub-shell missing\n> +\tgit checkout --quiet -b bmark &&\n> +\tgit remote -v &&\n> +\techo b >b &&\n> +\tgit add b &&\n> +\tgit commit -m b &&\n> +\tgit push origin bmark\n> +'\n> +\n>  test_done\n"},{"id":"237378","messageId":"10F8010F-96E2-45E0-B6D4-C3709AED3C28@quendi.de","threadId":"36263","inReplyTo":"532CA557.20007@web.de","subject":"Re: [PATCH v3] remote-hg: do not fail on invalid bookmarks","fromName":"Max Horn","fromEmail":"max@quendi.de","sentAt":"2014-03-21T21:44:11Z","receivedAt":"2014-03-21T21:44:11Z","isPatch":true,"sender":{"key":"max@quendi.de","avatar":"https://avatars.githubusercontent.com/u/241512?v=4"},"body":"Hi Torsten,\n\nOn 21.03.2014, at 21:47, Torsten Bögershausen <tboegi@web.de> wrote:\n\n> On 2014-03-21 12.36, Max Horn wrote:\n> All tests passed :-),\n\nExcellent.\n\n> thanks from my side.\n> comments inline, some are debatable\n\nThanks for having a close look and for the constructive feedback!\nUnfortunately, I won't have time to look into this for the next 7 days\nor so. I wouldn't mind if the patch gets queued with the changes you\nsuggest; but of course that might be a tad too much to ask for, so I'll\nalso be happy to do a \"proper\" re-roll, but then it has to wait a bit.\n\nCheers,\nMax\n\n"},{"id":"237382","messageId":"xmqq7g7nrxmv.fsf@gitster.dls.corp.google.com","threadId":"36263","inReplyTo":"10F8010F-96E2-45E0-B6D4-C3709AED3C28@quendi.de","subject":"Re: [PATCH v3] remote-hg: do not fail on invalid bookmarks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-21T22:32:24Z","receivedAt":"2014-03-21T22:32:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Horn <max@quendi.de> writes:\n\n> Hi Torsten,\n>\n> On 21.03.2014, at 21:47, Torsten Bögershausen <tboegi@web.de> wrote:\n>\n>> On 2014-03-21 12.36, Max Horn wrote:\n>> All tests passed :-),\n>\n> Excellent.\n>\n>> thanks from my side.\n>> comments inline, some are debatable\n>\n> Thanks for having a close look and for the constructive feedback!\n> Unfortunately, I won't have time to look into this for the next 7 days\n> or so. I wouldn't mind if the patch gets queued with the changes you\n> suggest; but of course that might be a tad too much to ask for, so I'll\n> also be happy to do a \"proper\" re-roll, but then it has to wait a bit.\n\nIn the meantime, I'll pile this on top as \"SQUASH???\".\n\nI am not sure how the original, which went into a subdirectory\ngitrepo that is to be cleaned with test_when_finished, was working.\nPerhaps it didn't clean and dug the trash directory hierarchy deeper\nand deeper, or something?\n\n\n contrib/remote-helpers/test-hg.sh | 80 +++++++++++++++++++++------------------\n 1 file changed, 43 insertions(+), 37 deletions(-)\n\ndiff --git a/contrib/remote-helpers/test-hg.sh b/contrib/remote-helpers/test-hg.sh\nindex 6925ca3..8834482 100755\n--- a/contrib/remote-helpers/test-hg.sh\n+++ b/contrib/remote-helpers/test-hg.sh\n@@ -694,68 +694,74 @@ test_expect_success 'remote double failed push' '\n test_expect_success 'clone remote with master null bookmark, then push to the bookmark' '\n \ttest_when_finished \"rm -rf gitrepo* hgrepo*\" &&\n \n-\t(\n \thg init hgrepo &&\n-\tcd hgrepo &&\n-\techo a >a &&\n-\thg add a &&\n-\thg commit -m a &&\n-\thg bookmark -r null master\n+\t(\n+\t\tcd hgrepo &&\n+\t\techo a >a &&\n+\t\thg add a &&\n+\t\thg commit -m a &&\n+\t\thg bookmark -r null master\n \t) &&\n \n \tgit clone \"hg::hgrepo\" gitrepo &&\n \tcheck gitrepo HEAD a &&\n-\tcd gitrepo &&\n-\tgit checkout --quiet -b master &&\n-\techo b >b &&\n-\tgit add b &&\n-\tgit commit -m b &&\n-\tgit push origin master\n+\t(\n+\t\tcd gitrepo &&\n+\t\tgit checkout --quiet -b master &&\n+\t\techo b >b &&\n+\t\tgit add b &&\n+\t\tgit commit -m b &&\n+\t\tgit push origin master\n+\t)\n '\n \n test_expect_success 'clone remote with default null bookmark, then push to the bookmark' '\n \ttest_when_finished \"rm -rf gitrepo* hgrepo*\" &&\n \n-\t(\n \thg init hgrepo &&\n-\tcd hgrepo &&\n-\techo a >a &&\n-\thg add a &&\n-\thg commit -m a &&\n-\thg bookmark -r null -f default\n+\t(\n+\t\tcd hgrepo &&\n+\t\techo a >a &&\n+\t\thg add a &&\n+\t\thg commit -m a &&\n+\t\thg bookmark -r null -f default\n \t) &&\n \n \tgit clone \"hg::hgrepo\" gitrepo &&\n \tcheck gitrepo HEAD a &&\n-\tcd gitrepo &&\n-\tgit checkout --quiet -b default &&\n-\techo b >b &&\n-\tgit add b &&\n-\tgit commit -m b &&\n-\tgit push origin default\n+\t(\n+\t\tcd gitrepo &&\n+\t\tgit checkout --quiet -b default &&\n+\t\techo b >b &&\n+\t\tgit add b &&\n+\t\tgit commit -m b &&\n+\t\tgit push origin default\n+\t)\n '\n \n test_expect_success 'clone remote with generic null bookmark, then push to the bookmark' '\n \ttest_when_finished \"rm -rf gitrepo* hgrepo*\" &&\n \n-\t(\n \thg init hgrepo &&\n-\tcd hgrepo &&\n-\techo a >a &&\n-\thg add a &&\n-\thg commit -m a &&\n-\thg bookmark -r null bmark\n+\t(\n+\t\tcd hgrepo &&\n+\t\techo a >a &&\n+\t\thg add a &&\n+\t\thg commit -m a &&\n+\t\thg bookmark -r null bmark\n \t) &&\n \n \tgit clone \"hg::hgrepo\" gitrepo &&\n \tcheck gitrepo HEAD a &&\n-\tcd gitrepo &&\n-\tgit checkout --quiet -b bmark &&\n-\tgit remote -v &&\n-\techo b >b &&\n-\tgit add b &&\n-\tgit commit -m b &&\n-\tgit push origin bmark\n+\t(\n+\t\tcd gitrepo &&\n+\t\tgit checkout --quiet -b bmark &&\n+\t\tgit remote -v &&\n+\t\techo b >b &&\n+\t\tgit add b &&\n+\t\tgit commit -m b &&\n+\t\tgit push origin bmark\n+\t)\n '\n \n test_done\n"},{"id":"237394","messageId":"532DBD41.6080008@web.de","threadId":"36263","inReplyTo":"xmqq7g7nrxmv.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3] remote-hg: do not fail on invalid bookmarks","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-03-22T16:41:37Z","receivedAt":"2014-03-22T16:41:37Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2014-03-21 23.32, Junio C Hamano wrote:\n> Max Horn <max@quendi.de> writes:\n> \n>> Hi Torsten,\n>>\n>> On 21.03.2014, at 21:47, Torsten Bögershausen <tboegi@web.de> wrote:\n>>\n>>> On 2014-03-21 12.36, Max Horn wrote:\n>>> All tests passed :-),\n>>\n>> Excellent.\n>>\n>>> thanks from my side.\n>>> comments inline, some are debatable\n>>\n>> Thanks for having a close look and for the constructive feedback!\n>> Unfortunately, I won't have time to look into this for the next 7 days\n>> or so. I wouldn't mind if the patch gets queued with the changes you\n>> suggest; but of course that might be a tad too much to ask for, so I'll\n>> also be happy to do a \"proper\" re-roll, but then it has to wait a bit.\n> \n> In the meantime, I'll pile this on top as \"SQUASH???\".\n\nTest OK under Linux and Mac,\n(so this is a little ACK).\n"}]}