v3remote-hg: do not fail on invalid bookmarks

> 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

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

> 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???".