Patchwork [3,of,8,V5] bundle2: add `bookmarks` part handler

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

Comments

Stanislau Hlebik - Sept. 16, 2016, 11:10 a.m.
# HG changeset patch
# User Stanislau Hlebik <stash@fb.com>
# Date 1473954996 25200
#      Thu Sep 15 08:56:36 2016 -0700
# Node ID 3456cba8a4879e74f3b928df321ce7e7c695fb4c
# Parent  f3fb030f0e4601561ac94137c7481694407db7b7
bundle2: add `bookmarks` part handler
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 1473954996 25200
> #      Thu Sep 15 08:56:36 2016 -0700
> # Node ID 3456cba8a4879e74f3b928df321ce7e7c695fb4c
> # Parent  f3fb030f0e4601561ac94137c7481694407db7b7
> bundle2: add `bookmarks` part handler
>
> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
> --- a/mercurial/bundle2.py
> +++ b/mercurial/bundle2.py
> @@ -154,8 +154,12 @@
>  import sys
>
>  from .i18n import _
> +from .node import (
> +    bin,
> +)
>  from . import (
>      changegroup,
> +    encoding,
>      error,
>      obsolete,
>      pushkey,
> @@ -1614,3 +1618,20 @@
>
>      cache.write()
>      op.ui.debug('applied %i hgtags fnodes cache entries\n' % count)
> +
> +@parthandler('bookmarks')
> +def handlebookmarks(op, inpart):


Please add some docstring to this part.

Greg, do you think we should also update the technical documentation 
specification of each part or should the docstring take care of this ?

> +    dec = encoding.tolocal
> +    bookmarks = {}
> +    for bookmarknode in inpart.read().splitlines():
> +        book, node = bookmarknode.rsplit(' ', 1)

We should probably stick on binary encoding for bookmark. This would 
avoid the need to encode/decode the hex and probably be more extensible 
in the future.

> +        bookmarks[dec(book)] = node
> +    if op.applybookmarks:
> +        for bookmark, node in bookmarks.items():
> +            if node:
> +                op.repo._bookmarks[bookmark] = bin(node)
> +            else:
> +                del op.repo._bookmarks[bookmark]
> +        op.repo._bookmarks.recordchange(op.gettransaction())

You wan to call gettransaction before doing any change. The pushrebase 
extension delay the locking until the transaction is actually fetched 
and this would expose you to a race here.

As you pointed on IRC, this is no longer trigger the pushkey hooks for 
bookmarks. This highlight the fact that:

(1) We should most probably introduces some hook dedicated to bookmark
(2) unfortunatly, we probably need to trigger the pushkey hook even if 
we are not using pushkey for BC reason :-(


> +    else:
> +        op.records.add('bookmarks', bookmarks)

We want to add record in all cases. The record are here to help other 
part of the push (eg: handler, hooks) to understand and collaborate with 
what happened.

Cheers,
Stanislau Hlebik - Oct. 5, 2016, 4:59 p.m.
What does binary encoding for bookmarks mean? Or you are talking about nodes encoding?

Thanks,
Stas

On 9/16/16, 4:53 PM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org> wrote:

    
    
    On 09/16/2016 01:10 PM, Stanislau Hlebik wrote:
    > # HG changeset patch

    > # User Stanislau Hlebik <stash@fb.com>

    > # Date 1473954996 25200

    > #      Thu Sep 15 08:56:36 2016 -0700

    > # Node ID 3456cba8a4879e74f3b928df321ce7e7c695fb4c

    > # Parent  f3fb030f0e4601561ac94137c7481694407db7b7

    > bundle2: add `bookmarks` part handler

    >

    > diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py

    > --- a/mercurial/bundle2.py

    > +++ b/mercurial/bundle2.py

    > @@ -154,8 +154,12 @@

    >  import sys

    >

    >  from .i18n import _

    > +from .node import (

    > +    bin,

    > +)

    >  from . import (

    >      changegroup,

    > +    encoding,

    >      error,

    >      obsolete,

    >      pushkey,

    > @@ -1614,3 +1618,20 @@

    >

    >      cache.write()

    >      op.ui.debug('applied %i hgtags fnodes cache entries\n' % count)

    > +

    > +@parthandler('bookmarks')

    > +def handlebookmarks(op, inpart):

    
    
    Please add some docstring to this part.
    
    Greg, do you think we should also update the technical documentation 
    specification of each part or should the docstring take care of this ?
    
    > +    dec = encoding.tolocal

    > +    bookmarks = {}

    > +    for bookmarknode in inpart.read().splitlines():

    > +        book, node = bookmarknode.rsplit(' ', 1)

    
    We should probably stick on binary encoding for bookmark. This would 
    avoid the need to encode/decode the hex and probably be more extensible 
    in the future.
    
    > +        bookmarks[dec(book)] = node

    > +    if op.applybookmarks:

    > +        for bookmark, node in bookmarks.items():

    > +            if node:

    > +                op.repo._bookmarks[bookmark] = bin(node)

    > +            else:

    > +                del op.repo._bookmarks[bookmark]

    > +        op.repo._bookmarks.recordchange(op.gettransaction())

    
    You wan to call gettransaction before doing any change. The pushrebase 
    extension delay the locking until the transaction is actually fetched 
    and this would expose you to a race here.
    
    As you pointed on IRC, this is no longer trigger the pushkey hooks for 
    bookmarks. This highlight the fact that:
    
    (1) We should most probably introduces some hook dedicated to bookmark
    (2) unfortunatly, we probably need to trigger the pushkey hook even if 
    we are not using pushkey for BC reason :-(
    
    
    > +    else:

    > +        op.records.add('bookmarks', bookmarks)

    
    We want to add record in all cases. The record are here to help other 
    part of the push (eg: handler, hooks) to understand and collaborate with 
    what happened.
    
    Cheers,
    
    -- 
    Pierre-Yves David
Stanislau Hlebik - Oct. 5, 2016, 5 p.m.
Calling prepushkey and pushkey hook from `bookmarks` part seems a bit hacky and my current local implementation is not very pleasant. I'll try to come up with smth better


On 10/5/16, 5:59 PM, "Stanislau Hlebik" <stash@fb.com> wrote:

    What does binary encoding for bookmarks mean? Or you are talking about nodes encoding?
    
    Thanks,
    Stas
    
    On 9/16/16, 4:53 PM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org> wrote:
    
        
        
        On 09/16/2016 01:10 PM, Stanislau Hlebik wrote:
        > # HG changeset patch

        > # User Stanislau Hlebik <stash@fb.com>

        > # Date 1473954996 25200

        > #      Thu Sep 15 08:56:36 2016 -0700

        > # Node ID 3456cba8a4879e74f3b928df321ce7e7c695fb4c

        > # Parent  f3fb030f0e4601561ac94137c7481694407db7b7

        > bundle2: add `bookmarks` part handler

        >

        > diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py

        > --- a/mercurial/bundle2.py

        > +++ b/mercurial/bundle2.py

        > @@ -154,8 +154,12 @@

        >  import sys

        >

        >  from .i18n import _

        > +from .node import (

        > +    bin,

        > +)

        >  from . import (

        >      changegroup,

        > +    encoding,

        >      error,

        >      obsolete,

        >      pushkey,

        > @@ -1614,3 +1618,20 @@

        >

        >      cache.write()

        >      op.ui.debug('applied %i hgtags fnodes cache entries\n' % count)

        > +

        > +@parthandler('bookmarks')

        > +def handlebookmarks(op, inpart):

        
        
        Please add some docstring to this part.
        
        Greg, do you think we should also update the technical documentation 
        specification of each part or should the docstring take care of this ?
        
        > +    dec = encoding.tolocal

        > +    bookmarks = {}

        > +    for bookmarknode in inpart.read().splitlines():

        > +        book, node = bookmarknode.rsplit(' ', 1)

        
        We should probably stick on binary encoding for bookmark. This would 
        avoid the need to encode/decode the hex and probably be more extensible 
        in the future.
        
        > +        bookmarks[dec(book)] = node

        > +    if op.applybookmarks:

        > +        for bookmark, node in bookmarks.items():

        > +            if node:

        > +                op.repo._bookmarks[bookmark] = bin(node)

        > +            else:

        > +                del op.repo._bookmarks[bookmark]

        > +        op.repo._bookmarks.recordchange(op.gettransaction())

        
        You wan to call gettransaction before doing any change. The pushrebase 
        extension delay the locking until the transaction is actually fetched 
        and this would expose you to a race here.
        
        As you pointed on IRC, this is no longer trigger the pushkey hooks for 
        bookmarks. This highlight the fact that:
        
        (1) We should most probably introduces some hook dedicated to bookmark
        (2) unfortunatly, we probably need to trigger the pushkey hook even if 
        we are not using pushkey for BC reason :-(
        
        
        > +    else:

        > +        op.records.add('bookmarks', bookmarks)

        
        We want to add record in all cases. The record are here to help other 
        part of the push (eg: handler, hooks) to understand and collaborate with 
        what happened.
        
        Cheers,
        
        -- 
        Pierre-Yves David
Pierre-Yves David - Oct. 6, 2016, 11:33 a.m.
On 10/05/2016 06:59 PM, Stanislau Hlebik wrote:
> What does binary encoding for bookmarks mean? Or you are talking about nodes encoding?

It means using a binary format instead of a textual one. Historically 
Mercurial have been using textual format a lot, but we have been moving 
to favoring binary ones in the later years.

A binary format for bookmarks could be:

   <20 bytes for nodes><4 (or 2?) bytes for length of 
bookmarks><bookmark name>

>
> Thanks,
> Stas
>
> On 9/16/16, 4:53 PM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org> wrote:
>
>
>
>     On 09/16/2016 01:10 PM, Stanislau Hlebik wrote:
>     > # HG changeset patch
>     > # User Stanislau Hlebik <stash@fb.com>
>     > # Date 1473954996 25200
>     > #      Thu Sep 15 08:56:36 2016 -0700
>     > # Node ID 3456cba8a4879e74f3b928df321ce7e7c695fb4c
>     > # Parent  f3fb030f0e4601561ac94137c7481694407db7b7
>     > bundle2: add `bookmarks` part handler
>     >
>     > diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
>     > --- a/mercurial/bundle2.py
>     > +++ b/mercurial/bundle2.py
>     > @@ -154,8 +154,12 @@
>     >  import sys
>     >
>     >  from .i18n import _
>     > +from .node import (
>     > +    bin,
>     > +)
>     >  from . import (
>     >      changegroup,
>     > +    encoding,
>     >      error,
>     >      obsolete,
>     >      pushkey,
>     > @@ -1614,3 +1618,20 @@
>     >
>     >      cache.write()
>     >      op.ui.debug('applied %i hgtags fnodes cache entries\n' % count)
>     > +
>     > +@parthandler('bookmarks')
>     > +def handlebookmarks(op, inpart):
>
>
>     Please add some docstring to this part.
>
>     Greg, do you think we should also update the technical documentation
>     specification of each part or should the docstring take care of this ?
>
>     > +    dec = encoding.tolocal
>     > +    bookmarks = {}
>     > +    for bookmarknode in inpart.read().splitlines():
>     > +        book, node = bookmarknode.rsplit(' ', 1)
>
>     We should probably stick on binary encoding for bookmark. This would
>     avoid the need to encode/decode the hex and probably be more extensible
>     in the future.
>
>     > +        bookmarks[dec(book)] = node
>     > +    if op.applybookmarks:
>     > +        for bookmark, node in bookmarks.items():
>     > +            if node:
>     > +                op.repo._bookmarks[bookmark] = bin(node)
>     > +            else:
>     > +                del op.repo._bookmarks[bookmark]
>     > +        op.repo._bookmarks.recordchange(op.gettransaction())
>
>     You wan to call gettransaction before doing any change. The pushrebase
>     extension delay the locking until the transaction is actually fetched
>     and this would expose you to a race here.
>
>     As you pointed on IRC, this is no longer trigger the pushkey hooks for
>     bookmarks. This highlight the fact that:
>
>     (1) We should most probably introduces some hook dedicated to bookmark
>     (2) unfortunatly, we probably need to trigger the pushkey hook even if
>     we are not using pushkey for BC reason :-(
>
>
>     > +    else:
>     > +        op.records.add('bookmarks', bookmarks)
>
>     We want to add record in all cases. The record are here to help other
>     part of the push (eg: handler, hooks) to understand and collaborate with
>     what happened.
>
>     Cheers,
>
>     --
>     Pierre-Yves David
>
>
Pierre-Yves David - Oct. 6, 2016, 11:34 a.m.
On 10/05/2016 07:00 PM, Stanislau Hlebik wrote:
> Calling prepushkey and pushkey hook from `bookmarks` part seems a bit hacky and my current local implementation is not very pleasant. I'll try to come up with smth better

I'm afraid we have to :-/ people enforcing some bookmark semantic 
through pushkey hooks would have that broken otherwise. Having to do 
this, that really does not makes me happy.

Cheers,

Patch

diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
--- a/mercurial/bundle2.py
+++ b/mercurial/bundle2.py
@@ -154,8 +154,12 @@ 
 import sys
 
 from .i18n import _
+from .node import (
+    bin,
+)
 from . import (
     changegroup,
+    encoding,
     error,
     obsolete,
     pushkey,
@@ -1614,3 +1618,20 @@ 
 
     cache.write()
     op.ui.debug('applied %i hgtags fnodes cache entries\n' % count)
+
+@parthandler('bookmarks')
+def handlebookmarks(op, inpart):
+    dec = encoding.tolocal
+    bookmarks = {}
+    for bookmarknode in inpart.read().splitlines():
+        book, node = bookmarknode.rsplit(' ', 1)
+        bookmarks[dec(book)] = node
+    if op.applybookmarks:
+        for bookmark, node in bookmarks.items():
+            if node:
+                op.repo._bookmarks[bookmark] = bin(node)
+            else:
+                del op.repo._bookmarks[bookmark]
+        op.repo._bookmarks.recordchange(op.gettransaction())
+    else:
+        op.records.add('bookmarks', bookmarks)