Patchwork [12,of,15,V2] bundle2: support a 'records' mode for the 'bookmarks' part

login
register
mail settings
Submitter Boris Feld
Date Nov. 2, 2017, 1:18 p.m.
Message ID <68bbec307c142b6b4189.1509628689@FB>
Download mbox | patch
Permalink /patch/25340/
State Accepted
Headers show

Comments

Boris Feld - Nov. 2, 2017, 1:18 p.m.
# HG changeset patch
# User Boris Feld <boris.feld@octobus.net>
# Date 1508246776 -7200
#      Tue Oct 17 15:26:16 2017 +0200
# Node ID 68bbec307c142b6b41893512b1c76320c87c2fa1
# Parent  bd3927325fe48e104b1627e5681ccd09a9a49e44
# EXP-Topic b2.bookmarks
# Available At https://bitbucket.org/octobus/mercurial-devel/
#              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 68bbec307c14
bundle2: support a 'records' mode for the 'bookmarks' part

In this mode, the bookmarks changes are record in the 'bundleoperation' records
instead of inflicted to the repository. This is necessary to use the part when
pulling.
Augie Fackler - Nov. 10, 2017, 10:43 p.m.
On Thu, Nov 02, 2017 at 02:18:09PM +0100, Boris Feld wrote:
> # HG changeset patch
> # User Boris Feld <boris.feld@octobus.net>
> # Date 1508246776 -7200
> #      Tue Oct 17 15:26:16 2017 +0200
> # Node ID 68bbec307c142b6b41893512b1c76320c87c2fa1
> # Parent  bd3927325fe48e104b1627e5681ccd09a9a49e44
> # EXP-Topic b2.bookmarks
> # Available At https://bitbucket.org/octobus/mercurial-devel/
> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 68bbec307c14
> bundle2: support a 'records' mode for the 'bookmarks' part
>
> In this mode, the bookmarks changes are record in the 'bundleoperation' records
> instead of inflicted to the repository. This is necessary to use the part when

s/inflicted/applied/?

> pulling.
>

I'm confused. Why do we not want to apply the part when we're pulling?
This log message could use some expansion.

>
> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
> --- a/mercurial/bundle2.py
> +++ b/mercurial/bundle2.py
> @@ -1897,40 +1897,55 @@ def handlepushkey(op, inpart):
>  def handlebookmark(op, inpart):
>      """transmit bookmark information
>
> -    The part contains binary encoded bookmark information. The bookmark
> -    information is applied as is to the unbundling repository. Make sure a
> -    'check:bookmarks' part is issued earlier to check for race condition in
> -    such update.
> +    The part contains binary encoded bookmark information.
> +
> +    The exact behavior of this part can be controlled by the 'bookmarks' mode
> +    on the bundle operation.
>
> -    This behavior is suitable for pushing. Semantic adjustment will be needed
> -    for pull.
> +    When mode is 'apply' (the default) the bookmark information is applied as
> +    is to the unbundling repository. Make sure a 'check:bookmarks' part is
> +    issued earlier to check for push races in such update. This behavior is
> +    suitable for pushing.
> +
> +    When mode is 'records', the information is recorded into the 'bookmarks'
> +    records of the bundle operation. This behavior is suitable for pulling.
>      """
>      changes = bookmarks.binarydecode(inpart)
>
> -    tr = op.gettransaction()
> -    bookstore = op.repo._bookmarks
> +    pushkeycompat = op.repo.ui.configbool('server', 'bookmarks-pushkey-compat')
> +    bookmarksmode = op.modes.get('bookmarks', 'apply')
>
> -    pushkeycompat = op.repo.ui.configbool('server', 'bookmarks-pushkey-compat')
> -    if pushkeycompat:
> -        allhooks = []
> +    if bookmarksmode == 'apply':
> +        tr = op.gettransaction()
> +        bookstore = op.repo._bookmarks
> +        if pushkeycompat:
> +            allhooks = []
> +            for book, node in changes:
> +                hookargs = tr.hookargs.copy()
> +                hookargs['pushkeycompat'] = '1'
> +                hookargs['namespace'] = 'bookmark'
> +                hookargs['key'] = book
> +                hookargs['old'] = nodemod.hex(bookstore.get(book, ''))
> +                hookargs['new'] = nodemod.hex(node if node is not None else '')
> +                allhooks.append(hookargs)
> +
> +            for hookargs in allhooks:
> +                op.repo.hook('prepushkey', throw=True, **hookargs)
> +
> +        bookstore.applychanges(op.repo, op.gettransaction(), changes)
> +
> +        if pushkeycompat:
> +            def runhook():
> +                for hookargs in allhooks:
> +                    op.repo.hook('prepushkey', **hookargs)
> +            op.repo._afterlock(runhook)
> +
> +    elif bookmarksmode == 'records':
>          for book, node in changes:
> -            hookargs = tr.hookargs.copy()
> -            hookargs['pushkeycompat'] = '1'
> -            hookargs['namespace'] = 'bookmark'
> -            hookargs['key'] = book
> -            hookargs['old'] = nodemod.hex(bookstore.get(book, ''))
> -            hookargs['new'] = nodemod.hex(node if node is not None else '')
> -            allhooks.append(hookargs)
> -        for hookargs in allhooks:
> -            op.repo.hook('prepushkey', throw=True, **hookargs)
> -
> -    bookstore.applychanges(op.repo, tr, changes)
> -
> -    if pushkeycompat:
> -        def runhook():
> -            for hookargs in allhooks:
> -                op.repo.hook('prepushkey', **hookargs)
> -        op.repo._afterlock(runhook)
> +            record = {'bookmark': book, 'node': node}
> +            op.records.add('bookmarks', record)
> +    else:
> +        raise error.ProgrammingError('unkown bookmark mode: %s' % bookmarksmode)
>
>  @parthandler('phase-heads')
>  def handlephases(op, inpart):
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Boris Feld - Nov. 13, 2017, 10:24 a.m.
On Fri, 2017-11-10 at 17:43 -0500, Augie Fackler wrote:
> On Thu, Nov 02, 2017 at 02:18:09PM +0100, Boris Feld wrote:
> > # HG changeset patch
> > # User Boris Feld <boris.feld@octobus.net>
> > # Date 1508246776 -7200
> > #      Tue Oct 17 15:26:16 2017 +0200
> > # Node ID 68bbec307c142b6b41893512b1c76320c87c2fa1
> > # Parent  bd3927325fe48e104b1627e5681ccd09a9a49e44
> > # EXP-Topic b2.bookmarks
> > # Available At https://bitbucket.org/octobus/mercurial-devel/
> > #              hg pull https://bitbucket.org/octobus/mercurial-deve
> > l/ -r 68bbec307c14
> > bundle2: support a 'records' mode for the 'bookmarks' part
> > 
> > In this mode, the bookmarks changes are record in the
> > 'bundleoperation' records
> > instead of inflicted to the repository. This is necessary to use
> > the part when
> 
> s/inflicted/applied/?
> 
> > pulling.
> > 
> 
> I'm confused. Why do we not want to apply the part when we're
> pulling?
> This log message could use some expansion.

When pulling, the remote bookmark value is not just applied -as-is-
into the local repository. There is an extra step to detect bookmark
divergence. The remote bookmarks data are stored until this processing
happens.

> 
> > 
> > diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
> > --- a/mercurial/bundle2.py
> > +++ b/mercurial/bundle2.py
> > @@ -1897,40 +1897,55 @@ def handlepushkey(op, inpart):
> >  def handlebookmark(op, inpart):
> >      """transmit bookmark information
> > 
> > -    The part contains binary encoded bookmark information. The
> > bookmark
> > -    information is applied as is to the unbundling repository.
> > Make sure a
> > -    'check:bookmarks' part is issued earlier to check for race
> > condition in
> > -    such update.
> > +    The part contains binary encoded bookmark information.
> > +
> > +    The exact behavior of this part can be controlled by the
> > 'bookmarks' mode
> > +    on the bundle operation.
> > 
> > -    This behavior is suitable for pushing. Semantic adjustment
> > will be needed
> > -    for pull.
> > +    When mode is 'apply' (the default) the bookmark information is
> > applied as
> > +    is to the unbundling repository. Make sure a 'check:bookmarks'
> > part is
> > +    issued earlier to check for push races in such update. This
> > behavior is
> > +    suitable for pushing.
> > +
> > +    When mode is 'records', the information is recorded into the
> > 'bookmarks'
> > +    records of the bundle operation. This behavior is suitable for
> > pulling.
> >      """
> >      changes = bookmarks.binarydecode(inpart)
> > 
> > -    tr = op.gettransaction()
> > -    bookstore = op.repo._bookmarks
> > +    pushkeycompat = op.repo.ui.configbool('server', 'bookmarks-
> > pushkey-compat')
> > +    bookmarksmode = op.modes.get('bookmarks', 'apply')
> > 
> > -    pushkeycompat = op.repo.ui.configbool('server', 'bookmarks-
> > pushkey-compat')
> > -    if pushkeycompat:
> > -        allhooks = []
> > +    if bookmarksmode == 'apply':
> > +        tr = op.gettransaction()
> > +        bookstore = op.repo._bookmarks
> > +        if pushkeycompat:
> > +            allhooks = []
> > +            for book, node in changes:
> > +                hookargs = tr.hookargs.copy()
> > +                hookargs['pushkeycompat'] = '1'
> > +                hookargs['namespace'] = 'bookmark'
> > +                hookargs['key'] = book
> > +                hookargs['old'] = nodemod.hex(bookstore.get(book,
> > ''))
> > +                hookargs['new'] = nodemod.hex(node if node is not
> > None else '')
> > +                allhooks.append(hookargs)
> > +
> > +            for hookargs in allhooks:
> > +                op.repo.hook('prepushkey', throw=True, **hookargs)
> > +
> > +        bookstore.applychanges(op.repo, op.gettransaction(),
> > changes)
> > +
> > +        if pushkeycompat:
> > +            def runhook():
> > +                for hookargs in allhooks:
> > +                    op.repo.hook('prepushkey', **hookargs)
> > +            op.repo._afterlock(runhook)
> > +
> > +    elif bookmarksmode == 'records':
> >          for book, node in changes:
> > -            hookargs = tr.hookargs.copy()
> > -            hookargs['pushkeycompat'] = '1'
> > -            hookargs['namespace'] = 'bookmark'
> > -            hookargs['key'] = book
> > -            hookargs['old'] = nodemod.hex(bookstore.get(book, ''))
> > -            hookargs['new'] = nodemod.hex(node if node is not None
> > else '')
> > -            allhooks.append(hookargs)
> > -        for hookargs in allhooks:
> > -            op.repo.hook('prepushkey', throw=True, **hookargs)
> > -
> > -    bookstore.applychanges(op.repo, tr, changes)
> > -
> > -    if pushkeycompat:
> > -        def runhook():
> > -            for hookargs in allhooks:
> > -                op.repo.hook('prepushkey', **hookargs)
> > -        op.repo._afterlock(runhook)
> > +            record = {'bookmark': book, 'node': node}
> > +            op.records.add('bookmarks', record)
> > +    else:
> > +        raise error.ProgrammingError('unkown bookmark mode: %s' %
> > bookmarksmode)
> > 
> >  @parthandler('phase-heads')
> >  def handlephases(op, inpart):
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Augie Fackler - Nov. 13, 2017, 11:16 p.m.
> On Nov 13, 2017, at 05:24, Boris Feld <boris.feld@octobus.net> wrote:
> 
> On Fri, 2017-11-10 at 17:43 -0500, Augie Fackler wrote:
>> On Thu, Nov 02, 2017 at 02:18:09PM +0100, Boris Feld wrote:
>>> # HG changeset patch
>>> # User Boris Feld <boris.feld@octobus.net>
>>> # Date 1508246776 -7200
>>> #      Tue Oct 17 15:26:16 2017 +0200
>>> # Node ID 68bbec307c142b6b41893512b1c76320c87c2fa1
>>> # Parent  bd3927325fe48e104b1627e5681ccd09a9a49e44
>>> # EXP-Topic b2.bookmarks
>>> # Available At https://bitbucket.org/octobus/mercurial-devel/
>>> #              hg pull https://bitbucket.org/octobus/mercurial-deve
>>> l/ -r 68bbec307c14
>>> bundle2: support a 'records' mode for the 'bookmarks' part
>>> 
>>> In this mode, the bookmarks changes are record in the
>>> 'bundleoperation' records
>>> instead of inflicted to the repository. This is necessary to use
>>> the part when
>> 
>> s/inflicted/applied/?
>> 
>>> pulling.
>>> 
>> 
>> I'm confused. Why do we not want to apply the part when we're
>> pulling?
>> This log message could use some expansion.
> 
> When pulling, the remote bookmark value is not just applied -as-is-
> into the local repository. There is an extra step to detect bookmark
> divergence. The remote bookmarks data are stored until this processing
> happens.

Ah. That wasn't obvious. Let's make sure that comment makes it into v3 of this series in the commit message.

Patch

diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
--- a/mercurial/bundle2.py
+++ b/mercurial/bundle2.py
@@ -1897,40 +1897,55 @@  def handlepushkey(op, inpart):
 def handlebookmark(op, inpart):
     """transmit bookmark information
 
-    The part contains binary encoded bookmark information. The bookmark
-    information is applied as is to the unbundling repository. Make sure a
-    'check:bookmarks' part is issued earlier to check for race condition in
-    such update.
+    The part contains binary encoded bookmark information.
+
+    The exact behavior of this part can be controlled by the 'bookmarks' mode
+    on the bundle operation.
 
-    This behavior is suitable for pushing. Semantic adjustment will be needed
-    for pull.
+    When mode is 'apply' (the default) the bookmark information is applied as
+    is to the unbundling repository. Make sure a 'check:bookmarks' part is
+    issued earlier to check for push races in such update. This behavior is
+    suitable for pushing.
+
+    When mode is 'records', the information is recorded into the 'bookmarks'
+    records of the bundle operation. This behavior is suitable for pulling.
     """
     changes = bookmarks.binarydecode(inpart)
 
-    tr = op.gettransaction()
-    bookstore = op.repo._bookmarks
+    pushkeycompat = op.repo.ui.configbool('server', 'bookmarks-pushkey-compat')
+    bookmarksmode = op.modes.get('bookmarks', 'apply')
 
-    pushkeycompat = op.repo.ui.configbool('server', 'bookmarks-pushkey-compat')
-    if pushkeycompat:
-        allhooks = []
+    if bookmarksmode == 'apply':
+        tr = op.gettransaction()
+        bookstore = op.repo._bookmarks
+        if pushkeycompat:
+            allhooks = []
+            for book, node in changes:
+                hookargs = tr.hookargs.copy()
+                hookargs['pushkeycompat'] = '1'
+                hookargs['namespace'] = 'bookmark'
+                hookargs['key'] = book
+                hookargs['old'] = nodemod.hex(bookstore.get(book, ''))
+                hookargs['new'] = nodemod.hex(node if node is not None else '')
+                allhooks.append(hookargs)
+
+            for hookargs in allhooks:
+                op.repo.hook('prepushkey', throw=True, **hookargs)
+
+        bookstore.applychanges(op.repo, op.gettransaction(), changes)
+
+        if pushkeycompat:
+            def runhook():
+                for hookargs in allhooks:
+                    op.repo.hook('prepushkey', **hookargs)
+            op.repo._afterlock(runhook)
+
+    elif bookmarksmode == 'records':
         for book, node in changes:
-            hookargs = tr.hookargs.copy()
-            hookargs['pushkeycompat'] = '1'
-            hookargs['namespace'] = 'bookmark'
-            hookargs['key'] = book
-            hookargs['old'] = nodemod.hex(bookstore.get(book, ''))
-            hookargs['new'] = nodemod.hex(node if node is not None else '')
-            allhooks.append(hookargs)
-        for hookargs in allhooks:
-            op.repo.hook('prepushkey', throw=True, **hookargs)
-
-    bookstore.applychanges(op.repo, tr, changes)
-
-    if pushkeycompat:
-        def runhook():
-            for hookargs in allhooks:
-                op.repo.hook('prepushkey', **hookargs)
-        op.repo._afterlock(runhook)
+            record = {'bookmark': book, 'node': node}
+            op.records.add('bookmarks', record)
+    else:
+        raise error.ProgrammingError('unkown bookmark mode: %s' % bookmarksmode)
 
 @parthandler('phase-heads')
 def handlephases(op, inpart):