Patchwork [4,of,8,V5] bundle2: add check:bookmarks part handler

login
register
mail settings
Submitter Stanislau Hlebik
Date Sept. 16, 2016, 11:10 a.m.
Message ID <7dd18199a5c4e890b634.1474024232@dev1918.lla1.facebook.com>
Download mbox | patch
Permalink /patch/16643/
State Accepted
Headers show

Comments

Stanislau Hlebik - Sept. 16, 2016, 11:10 a.m.
# HG changeset patch
# User Stanislau Hlebik <stash@fb.com>
# Date 1473955024 25200
#      Thu Sep 15 08:57:04 2016 -0700
# Node ID 7dd18199a5c4e890b634312684a1a05db2fbaac9
# Parent  3456cba8a4879e74f3b928df321ce7e7c695fb4c
bundle2: add check:bookmarks part handler

New `bookmark` part doesn't use pushkey infrastructure and doesn't prevent
bookmarks race conditions. So we need new a part handler similar to check:heads
that will check bookmark race conditions during push.
Pierre-Yves David - Sept. 16, 2016, 3:53 p.m.
On 09/16/2016 01:10 PM, Stanislau Hlebik wrote:
> # HG changeset patch
> # User Stanislau Hlebik <stash@fb.com>
> # Date 1473955024 25200
> #      Thu Sep 15 08:57:04 2016 -0700
> # Node ID 7dd18199a5c4e890b634312684a1a05db2fbaac9
> # Parent  3456cba8a4879e74f3b928df321ce7e7c695fb4c
> bundle2: add check:bookmarks part handler
>
> New `bookmark` part doesn't use pushkey infrastructure and doesn't prevent
> bookmarks race conditions. So we need new a part handler similar to check:heads
> that will check bookmark race conditions during push.
>
> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
> --- a/mercurial/bundle2.py
> +++ b/mercurial/bundle2.py
> @@ -156,6 +156,7 @@
>  from .i18n import _
>  from .node import (
>      bin,
> +    hex,
>  )
>  from . import (
>      changegroup,
> @@ -1469,6 +1470,21 @@
>          raise error.PushRaced('repository changed while pushing - '
>                                'please try again')
>
> +@parthandler('check:bookmarks')
> +def handlecheckbookmarks(op, inpart):
> +    """check that bookmarks did not change
> +
> +    This is used to detect a push race when using unbundle.
> +    """

1) same feedback about having more technical documentation
2) same question to greg about having this in the technical 
documentation. Given that two "disconnected" parts collaborate here, 
this probably mean we need them in the technical documentation.

> +    dec = encoding.tolocal
> +    for bookmarkoldnode in inpart.read().splitlines():
> +        bookmark, oldnode = bookmarkoldnode.rsplit(' ', 1)

Same feedback about using binary encoding directly.

> +        bookmark = dec(bookmark)
> +        marks = op.repo._bookmarks
> +        if bookmark not in marks or hex(marks[bookmark]) != oldnode:
> +            raise error.PushRaced('repository changed while pushing - '
> +                                  'please try again')

This is a bit outside of the scope of this series, but we might want to 
improve the accuracy of these error.

Cheers,

Patch

diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
--- a/mercurial/bundle2.py
+++ b/mercurial/bundle2.py
@@ -156,6 +156,7 @@ 
 from .i18n import _
 from .node import (
     bin,
+    hex,
 )
 from . import (
     changegroup,
@@ -1469,6 +1470,21 @@ 
         raise error.PushRaced('repository changed while pushing - '
                               'please try again')
 
+@parthandler('check:bookmarks')
+def handlecheckbookmarks(op, inpart):
+    """check that bookmarks did not change
+
+    This is used to detect a push race when using unbundle.
+    """
+    dec = encoding.tolocal
+    for bookmarkoldnode in inpart.read().splitlines():
+        bookmark, oldnode = bookmarkoldnode.rsplit(' ', 1)
+        bookmark = dec(bookmark)
+        marks = op.repo._bookmarks
+        if bookmark not in marks or hex(marks[bookmark]) != oldnode:
+            raise error.PushRaced('repository changed while pushing - '
+                                  'please try again')
+
 @parthandler('output')
 def handleoutput(op, inpart):
     """forward output captured on the server to the client"""