[PATCH v3] remote-hg: do not fail on invalid bookmarks

[PATCH v3] remote-hg: do not fail on invalid bookmarks

[Max Horn]

Mercurial can have bookmarks pointing to "nullid" (the empty root
revision), while Git can not have references to it. When cloning or
fetching from a Mercurial repository that has such a bookmark, the
import failed because git-remote-hg was not be able to create the
corresponding reference.

Warn the user about the invalid reference, and do not advertise these
bookmarks as head refs, but otherwise continue the import. In
particular, we still keep track of the fact that the remote repository
has a bookmark of the given name, in case the user wants to modify that
bookmark.

Also add some test cases for this issue.

Reported-by: Antoine Pelisse <apelisse@gmail.com>
Signed-off-by: Max Horn <max@quendi.de>
---
This is a different fix than in my previous attempts. I thought
a bit more about the issue, and determined that the previous fix,
while working, was not really correct: It is wrong to
treat nullid bookmarks as if they are non-existent; if e.g.
the user wants to modify the bookmark from git, we need to
into account that the remote already has a bookmark with that name.
Indeed, I extended the new test cases to cover this aspect.
With the previous fix, the new tests would fail upon pushing,
with the new one, they work.

 contrib/remote-helpers/git-remote-hg |  5 ++-
 contrib/remote-helpers/test-hg.sh    | 67 ++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
index eb89ef6..36b5261 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -643,7 +643,10 @@ def do_list(parser):
             print "? refs/heads/branches/%s" % gitref(branch)
 
     for bmark in bmarks:
-        print "? refs/heads/%s" % gitref(bmark)
+        if  bmarks[bmark].hex() == '0000000000000000000000000000000000000000':
+            warn("Ignoring invalid bookmark '%s'", bmark)
+        else:
+            print "? refs/heads/%s" % gitref(bmark)
 
     for tag, node in repo.tagslist():
         if tag == 'tip':
diff --git a/contrib/remote-helpers/test-hg.sh b/contrib/remote-helpers/test-hg.sh
index a933b1e..f5d0d97 100755
--- a/contrib/remote-helpers/test-hg.sh
+++ b/contrib/remote-helpers/test-hg.sh
@@ -772,4 +772,71 @@ test_expect_success 'remote double failed push' '
 	)
 '
 
+test_expect_success 'clone remote with master null bookmark, then push to the bookmark' '
+	test_when_finished "rm -rf gitrepo* hgrepo*" &&
+
+	(
+	hg init hgrepo &&
+	cd hgrepo &&
+	echo a >a &&
+	hg add a &&
+	hg commit -m a &&
+	hg bookmark -r null master
+	) &&
+
+	git clone "hg::hgrepo" gitrepo &&
+	check gitrepo HEAD a &&
+	cd gitrepo &&
+	git checkout --quiet -b master &&
+	echo b >b &&
+	git add b &&
+	git commit -m b &&
+	git push origin master
+'
+
+test_expect_success 'clone remote with default null bookmark, then push to the bookmark' '
+	test_when_finished "rm -rf gitrepo* hgrepo*" &&
+
+	(
+	hg init hgrepo &&
+	cd hgrepo &&
+	echo a >a &&
+	hg add a &&
+	hg commit -m a &&
+	hg bookmark -r null -f default
+	) &&
+
+	git clone "hg::hgrepo" gitrepo &&
+	check gitrepo HEAD a &&
+	cd gitrepo &&
+	git checkout --quiet -b default &&
+	echo b >b &&
+	git add b &&
+	git commit -m b &&
+	git push origin default
+'
+
+test_expect_success 'clone remote with generic null bookmark, then push to the bookmark' '
+	test_when_finished "rm -rf gitrepo* hgrepo*" &&
+
+	(
+	hg init hgrepo &&
+	cd hgrepo &&
+	echo a >a &&
+	hg add a &&
+	hg commit -m a &&
+	hg bookmark -r null bmark
+	) &&
+
+	git clone "hg::hgrepo" gitrepo &&
+	check gitrepo HEAD a &&
+	cd gitrepo &&
+	git checkout --quiet -b bmark &&
+	git remote -v &&
+	echo b >b &&
+	git add b &&
+	git commit -m b &&
+	git push origin bmark
+'
+
 test_done
-- 
1.9.0.7.ga299b13

Re: [PATCH v3] remote-hg: do not fail on invalid bookmarks

[Torsten Bögershausen]

On 2014-03-21 12.36, Max Horn wrote:
All tests passed :-), thanks from my side.
comments inline, some are debatable
> Mercurial can have bookmarks pointing to "nullid" (the empty root
> revision), while Git can not have references to it. When cloning or
> fetching from a Mercurial repository that has such a bookmark, the
> import failed because git-remote-hg was not be able to create the
> corresponding reference.
>
> Warn the user about the invalid reference, and do not advertise these
> bookmarks as head refs, but otherwise continue the import. In
> particular, we still keep track of the fact that the remote repository
> has a bookmark of the given name, in case the user wants to modify that
> bookmark.
>
> Also add some test cases for this issue.
s/some test cases/test cases/
>
> Reported-by: Antoine Pelisse <apelisse@gmail.com>
> Signed-off-by: Max Horn <max@quendi.de>
> ---
> This is a different fix than in my previous attempts. I thought
> a bit more about the issue, and determined that the previous fix,
> while working, was not really correct: It is wrong to
> treat nullid bookmarks as if they are non-existent; if e.g.
> the user wants to modify the bookmark from git, we need to
> into account that the remote already has a bookmark with that name.
> Indeed, I extended the new test cases to cover this aspect.
> With the previous fix, the new tests would fail upon pushing,
> with the new one, they work.
>
>  contrib/remote-helpers/git-remote-hg |  5 ++-
>  contrib/remote-helpers/test-hg.sh    | 67 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 71 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
> index eb89ef6..36b5261 100755
> --- a/contrib/remote-helpers/git-remote-hg
> +++ b/contrib/remote-helpers/git-remote-hg
> @@ -643,7 +643,10 @@ def do_list(parser):
>              print "? refs/heads/branches/%s" % gitref(branch)
>  
>      for bmark in bmarks:
> -        print "? refs/heads/%s" % gitref(bmark)
> +        if  bmarks[bmark].hex() == '0000000000000000000000000000000000000000':
> +            warn("Ignoring invalid bookmark '%s'", bmark)
> +        else:
> +            print "? refs/heads/%s" % gitref(bmark)
>  
>      for tag, node in repo.tagslist():
>          if tag == 'tip':
> diff --git a/contrib/remote-helpers/test-hg.sh b/contrib/remote-helpers/test-hg.sh
> index a933b1e..f5d0d97 100755
> --- a/contrib/remote-helpers/test-hg.sh
> +++ b/contrib/remote-helpers/test-hg.sh
> @@ -772,4 +772,71 @@ test_expect_success 'remote double failed push' '
>  	)
>  '
>  
> +test_expect_success 'clone remote with master null bookmark, then push to the bookmark' '
> +	test_when_finished "rm -rf gitrepo* hgrepo*" &&
> +
> +	(
> +	hg init hgrepo &&
> +	cd hgrepo &&
Minor:
We can change the order here, to make the "cd hgrepo" the first line in the subshell:

+	hg init hgrepo &&
+	(
+	cd hgrepo &&


> +	echo a >a &&
> +	hg add a &&
> +	hg commit -m a &&
> +	hg bookmark -r null master
> +	) &&
> +
> +	git clone "hg::hgrepo" gitrepo &&
> +	check gitrepo HEAD a &&
And here we do "cd", and this should be done in a subshell
> +	cd gitrepo &&
> +	git checkout --quiet -b master &&
> +	echo b >b &&
> +	git add b &&
> +	git commit -m b &&
> +	git push origin master
> +'
> +
> +test_expect_success 'clone remote with default null bookmark, then push to the bookmark' '
> +	test_when_finished "rm -rf gitrepo* hgrepo*" &&
> +
> +	(
> +	hg init hgrepo &&
> +	cd hgrepo &&
(Same minor as above)
> +	echo a >a &&
> +	hg add a &&
> +	hg commit -m a &&
> +	hg bookmark -r null -f default
> +	) &&
> +
> +	git clone "hg::hgrepo" gitrepo &&
> +	check gitrepo HEAD a &&
> +	cd gitrepo &&
> +	git checkout --quiet -b default &&
> +	echo b >b &&
> +	git add b &&
> +	git commit -m b &&
> +	git push origin default
> +'
> +
> +test_expect_success 'clone remote with generic null bookmark, then push to the bookmark' '
> +	test_when_finished "rm -rf gitrepo* hgrepo*" &&
> +
> +	(
> +	hg init hgrepo &&
> +	cd hgrepo &&
(Same as above)
> +	echo a >a &&
> +	hg add a &&
> +	hg commit -m a &&
> +	hg bookmark -r null bmark
> +	) &&
> +
> +	git clone "hg::hgrepo" gitrepo &&
> +	check gitrepo HEAD a &&
> +	cd gitrepo &&
Sub-shell missing
> +	git checkout --quiet -b bmark &&
> +	git remote -v &&
> +	echo b >b &&
> +	git add b &&
> +	git commit -m b &&
> +	git push origin bmark
> +'
> +
>  test_done

Re: [PATCH v3] remote-hg: do not fail on invalid bookmarks

[Max Horn]

[-- Attachment #1: Type: text/plain, Size: 605 bytes --]

Hi Torsten,

On 21.03.2014, at 21:47, Torsten Bögershausen <tboegi@web.de> wrote:

> On 2014-03-21 12.36, Max Horn wrote:
> All tests passed :-),

Excellent.

> thanks from my side.
> comments inline, some are debatable

Thanks for having a close look and for the constructive feedback!
Unfortunately, I won't have time to look into this for the next 7 days
or so. I wouldn't mind if the patch gets queued with the changes you
suggest; but of course that might be a tad too much to ask for, so I'll
also be happy to do a "proper" re-roll, but then it has to wait a bit.

Cheers,
Max


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 235 bytes --]

Re: [PATCH v3] remote-hg: do not fail on invalid bookmarks

[Junio C Hamano]

Max Horn <max@quendi.de> writes:

> Hi Torsten,
>
> On 21.03.2014, at 21:47, Torsten Bögershausen <tboegi@web.de> wrote:
>
>> On 2014-03-21 12.36, Max Horn wrote:
>> All tests passed :-),
>
> Excellent.
>
>> thanks from my side.
>> comments inline, some are debatable
>
> Thanks for having a close look and for the constructive feedback!
> Unfortunately, I won't have time to look into this for the next 7 days
> or so. I wouldn't mind if the patch gets queued with the changes you
> suggest; but of course that might be a tad too much to ask for, so I'll
> also be happy to do a "proper" re-roll, but then it has to wait a bit.

In the meantime, I'll pile this on top as "SQUASH???".

I am not sure how the original, which went into a subdirectory
gitrepo that is to be cleaned with test_when_finished, was working.
Perhaps it didn't clean and dug the trash directory hierarchy deeper
and deeper, or something?


 contrib/remote-helpers/test-hg.sh | 80 +++++++++++++++++++++------------------
 1 file changed, 43 insertions(+), 37 deletions(-)

diff --git a/contrib/remote-helpers/test-hg.sh b/contrib/remote-helpers/test-hg.sh
index 6925ca3..8834482 100755
--- a/contrib/remote-helpers/test-hg.sh
+++ b/contrib/remote-helpers/test-hg.sh
@@ -694,68 +694,74 @@ test_expect_success 'remote double failed push' '
 test_expect_success 'clone remote with master null bookmark, then push to the bookmark' '
 	test_when_finished "rm -rf gitrepo* hgrepo*" &&
 
-	(
 	hg init hgrepo &&
-	cd hgrepo &&
-	echo a >a &&
-	hg add a &&
-	hg commit -m a &&
-	hg bookmark -r null master
+	(
+		cd hgrepo &&
+		echo a >a &&
+		hg add a &&
+		hg commit -m a &&
+		hg bookmark -r null master
 	) &&
 
 	git clone "hg::hgrepo" gitrepo &&
 	check gitrepo HEAD a &&
-	cd gitrepo &&
-	git checkout --quiet -b master &&
-	echo b >b &&
-	git add b &&
-	git commit -m b &&
-	git push origin master
+	(
+		cd gitrepo &&
+		git checkout --quiet -b master &&
+		echo b >b &&
+		git add b &&
+		git commit -m b &&
+		git push origin master
+	)
 '
 
 test_expect_success 'clone remote with default null bookmark, then push to the bookmark' '
 	test_when_finished "rm -rf gitrepo* hgrepo*" &&
 
-	(
 	hg init hgrepo &&
-	cd hgrepo &&
-	echo a >a &&
-	hg add a &&
-	hg commit -m a &&
-	hg bookmark -r null -f default
+	(
+		cd hgrepo &&
+		echo a >a &&
+		hg add a &&
+		hg commit -m a &&
+		hg bookmark -r null -f default
 	) &&
 
 	git clone "hg::hgrepo" gitrepo &&
 	check gitrepo HEAD a &&
-	cd gitrepo &&
-	git checkout --quiet -b default &&
-	echo b >b &&
-	git add b &&
-	git commit -m b &&
-	git push origin default
+	(
+		cd gitrepo &&
+		git checkout --quiet -b default &&
+		echo b >b &&
+		git add b &&
+		git commit -m b &&
+		git push origin default
+	)
 '
 
 test_expect_success 'clone remote with generic null bookmark, then push to the bookmark' '
 	test_when_finished "rm -rf gitrepo* hgrepo*" &&
 
-	(
 	hg init hgrepo &&
-	cd hgrepo &&
-	echo a >a &&
-	hg add a &&
-	hg commit -m a &&
-	hg bookmark -r null bmark
+	(
+		cd hgrepo &&
+		echo a >a &&
+		hg add a &&
+		hg commit -m a &&
+		hg bookmark -r null bmark
 	) &&
 
 	git clone "hg::hgrepo" gitrepo &&
 	check gitrepo HEAD a &&
-	cd gitrepo &&
-	git checkout --quiet -b bmark &&
-	git remote -v &&
-	echo b >b &&
-	git add b &&
-	git commit -m b &&
-	git push origin bmark
+	(
+		cd gitrepo &&
+		git checkout --quiet -b bmark &&
+		git remote -v &&
+		echo b >b &&
+		git add b &&
+		git commit -m b &&
+		git push origin bmark
+	)
 '
 
 test_done

Re: [PATCH v3] remote-hg: do not fail on invalid bookmarks

[Torsten Bögershausen]

On 2014-03-21 23.32, Junio C Hamano wrote:
> Max Horn <max@quendi.de> writes:
> 
>> Hi Torsten,
>>
>> On 21.03.2014, at 21:47, Torsten Bögershausen <tboegi@web.de> wrote:
>>
>>> On 2014-03-21 12.36, Max Horn wrote:
>>> All tests passed :-),
>>
>> Excellent.
>>
>>> thanks from my side.
>>> comments inline, some are debatable
>>
>> Thanks for having a close look and for the constructive feedback!
>> Unfortunately, I won't have time to look into this for the next 7 days
>> or so. I wouldn't mind if the patch gets queued with the changes you
>> suggest; but of course that might be a tad too much to ask for, so I'll
>> also be happy to do a "proper" re-roll, but then it has to wait a bit.
> 
> In the meantime, I'll pile this on top as "SQUASH???".

Test OK under Linux and Mac,
(so this is a little ACK).