Patchwork [01,of,15] pull: store binary node in pullop.remotebookmarks

login
register
mail settings
Submitter Boris Feld
Date Oct. 18, 2017, 4:09 p.m.
Message ID <a450dc69ef95073af4d4.1508342993@FB>
Download mbox | patch
Permalink /patch/25184/
State Accepted
Headers show

Comments

Boris Feld - Oct. 18, 2017, 4:09 p.m.
# HG changeset patch
# User Boris Feld <boris.feld@octobus.net>
# Date 1508230905 -7200
#      Tue Oct 17 11:01:45 2017 +0200
# Node ID a450dc69ef95073af4d4ece34e251ed0bb5b808f
# Parent  537de0b14030868e3e850ae388b08f88cabc88e8
# EXP-Topic b2.bookmarks
# Available At https://bitbucket.org/octobus/mercurial-devel/
#              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r a450dc69ef95
pull: store binary node in pullop.remotebookmarks

The internal representation of bookmark value is binary. The fact we stored
'hex' was an implementation detail from using pushkey.

We move the values in 'pullop.remotebookmarks' to binary before adding a way to
exchange bookmarks not based on pushkey.
Gregory Szorc - Oct. 18, 2017, 4:59 p.m.
On Wed, Oct 18, 2017 at 6:09 PM, Boris Feld <boris.feld@octobus.net> wrote:

> # HG changeset patch
> # User Boris Feld <boris.feld@octobus.net>
> # Date 1508230905 -7200
> #      Tue Oct 17 11:01:45 2017 +0200
> # Node ID a450dc69ef95073af4d4ece34e251ed0bb5b808f
> # Parent  537de0b14030868e3e850ae388b08f88cabc88e8
> # EXP-Topic b2.bookmarks
> # Available At https://bitbucket.org/octobus/mercurial-devel/
> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r
> a450dc69ef95
> pull: store binary node in pullop.remotebookmarks
>
> The internal representation of bookmark value is binary. The fact we stored
> 'hex' was an implementation detail from using pushkey.
>
> We move the values in 'pullop.remotebookmarks' to binary before adding a
> way to
> exchange bookmarks not based on pushkey.
>

Queued part 1 (with "API" added to the commit message).


>
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -3911,12 +3911,13 @@
>              # not ending up with the name of the bookmark because of a
> race
>              # condition on the server. (See issue 4689 for details)
>              remotebookmarks = other.listkeys('bookmarks')
> +            remotebookmarks = bookmarks.unhexlifybookmarks(
> remotebookmarks)
>              pullopargs['remotebookmarks'] = remotebookmarks
>              for b in opts['bookmark']:
>                  b = repo._bookmarks.expandname(b)
>                  if b not in remotebookmarks:
>                      raise error.Abort(_('remote bookmark %s not found!')
> % b)
> -                revs.append(remotebookmarks[b])
> +                revs.append(hex(remotebookmarks[b]))
>
>          if revs:
>              try:
> diff --git a/mercurial/exchange.py b/mercurial/exchange.py
> --- a/mercurial/exchange.py
> +++ b/mercurial/exchange.py
> @@ -1348,7 +1348,8 @@
>          # all known bundle2 servers now support listkeys, but lets be
> nice with
>          # new implementation.
>          return
> -    pullop.remotebookmarks = pullop.remote.listkeys('bookmarks')
> +    books = pullop.remote.listkeys('bookmarks')
> +    pullop.remotebookmarks = bookmod.unhexlifybookmarks(books)
>
>
>  @pulldiscovery('changegroup')
> @@ -1459,7 +1460,7 @@
>      # processing bookmark update
>      for namespace, value in op.records['listkeys']:
>          if namespace == 'bookmarks':
> -            pullop.remotebookmarks = value
> +            pullop.remotebookmarks = bookmod.unhexlifybookmarks(value)
>
>      # bookmark data were either already there or pulled in the bundle
>      if pullop.remotebookmarks is not None:
> @@ -1552,7 +1553,6 @@
>      pullop.stepsdone.add('bookmarks')
>      repo = pullop.repo
>      remotebookmarks = pullop.remotebookmarks
> -    remotebookmarks = bookmod.unhexlifybookmarks(remotebookmarks)
>      bookmod.updatefromremote(repo.ui, repo, remotebookmarks,
>                               pullop.remote.url(),
>                               pullop.gettransaction,
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Gregory Szorc - Oct. 18, 2017, 5:25 p.m.
On Wed, Oct 18, 2017 at 6:09 PM, Boris Feld <boris.feld@octobus.net> wrote:

> # HG changeset patch
> # User Boris Feld <boris.feld@octobus.net>
> # Date 1508230905 -7200
> #      Tue Oct 17 11:01:45 2017 +0200
> # Node ID a450dc69ef95073af4d4ece34e251ed0bb5b808f
> # Parent  537de0b14030868e3e850ae388b08f88cabc88e8
> # EXP-Topic b2.bookmarks
> # Available At https://bitbucket.org/octobus/mercurial-devel/
> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r
> a450dc69ef95
> pull: store binary node in pullop.remotebookmarks
>
> The internal representation of bookmark value is binary. The fact we stored
> 'hex' was an implementation detail from using pushkey.
>
> We move the values in 'pullop.remotebookmarks' to binary before adding a
> way to
> exchange bookmarks not based on pushkey.
>

The overall approach seems reasonable and works. But the wire protocol is
one of those things that you need to support forever. So it is important
that any changes get strict scrutiny.

General questions/feedback:

* The series does the simplest thing and basically copies the old bookmarks
logic to bundle2. What are your thoughts on changing the structure a bit?
e.g. instead of separate check and set bundle parts, what about a single
part that records the old state of bookmarks? We'd need to send this first
to facilitate aborting early on consistency mismatch, of course. I'm not
claiming one approach is better: I'm just throwing out an alternative to
consider.

* There are a number of places that could benefit from early return control
flow.

* I believe after this series we now have both a "bookmarks" query string
argument *and* a "bookmarks" entry in the listkeys value for bundle
requests. Is this redundancy needed? Could you please document any new
arguments in wireproto.txt.

* The config option related to pushkey hooks firing is seemingly very
low-level. I'm tempted to omit the option and instead establish a BC
schedule to drop the old behavior (assuming we don't want to support it
forever).

* While we're here, a feature I've long wanted to implement is `hg pull
--mirror`. Please spend a few seconds contemplating if that would require
any wire protocol changes to support for bookmarks exchange. If so, we may
want to implement it now so we don't have to implement more "protocol
negotiation" later.

* Do we have sufficient test coverage around force pushing for both the old
and new style of bookmarks exchange? I could see that as one of those
things that easily regresses unless we're actively testing both variations.

* Landing wire protocol changes just before the freeze is a bit scary. I
know this is a major bug and I want to see it fixed as well. But the
schedule is concerning. I'd feel much more comfortable if this were behind
a config knob until 4.5.


>
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -3911,12 +3911,13 @@
>              # not ending up with the name of the bookmark because of a
> race
>              # condition on the server. (See issue 4689 for details)
>              remotebookmarks = other.listkeys('bookmarks')
> +            remotebookmarks = bookmarks.unhexlifybookmarks(r
> emotebookmarks)
>              pullopargs['remotebookmarks'] = remotebookmarks
>              for b in opts['bookmark']:
>                  b = repo._bookmarks.expandname(b)
>                  if b not in remotebookmarks:
>                      raise error.Abort(_('remote bookmark %s not found!')
> % b)
> -                revs.append(remotebookmarks[b])
> +                revs.append(hex(remotebookmarks[b]))
>
>          if revs:
>              try:
> diff --git a/mercurial/exchange.py b/mercurial/exchange.py
> --- a/mercurial/exchange.py
> +++ b/mercurial/exchange.py
> @@ -1348,7 +1348,8 @@
>          # all known bundle2 servers now support listkeys, but lets be
> nice with
>          # new implementation.
>          return
> -    pullop.remotebookmarks = pullop.remote.listkeys('bookmarks')
> +    books = pullop.remote.listkeys('bookmarks')
> +    pullop.remotebookmarks = bookmod.unhexlifybookmarks(books)
>
>
>  @pulldiscovery('changegroup')
> @@ -1459,7 +1460,7 @@
>      # processing bookmark update
>      for namespace, value in op.records['listkeys']:
>          if namespace == 'bookmarks':
> -            pullop.remotebookmarks = value
> +            pullop.remotebookmarks = bookmod.unhexlifybookmarks(value)
>
>      # bookmark data were either already there or pulled in the bundle
>      if pullop.remotebookmarks is not None:
> @@ -1552,7 +1553,6 @@
>      pullop.stepsdone.add('bookmarks')
>      repo = pullop.repo
>      remotebookmarks = pullop.remotebookmarks
> -    remotebookmarks = bookmod.unhexlifybookmarks(remotebookmarks)
>      bookmod.updatefromremote(repo.ui, repo, remotebookmarks,
>                               pullop.remote.url(),
>                               pullop.gettransaction,
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Boris Feld - Oct. 23, 2017, 2:26 p.m.
On Wed, 2017-10-18 at 19:25 +0200, Gregory Szorc wrote:
> On Wed, Oct 18, 2017 at 6:09 PM, Boris Feld <boris.feld@octobus.net>
> wrote:
> > # HG changeset patch
> > # User Boris Feld <boris.feld@octobus.net>
> > # Date 1508230905 -7200
> > #      Tue Oct 17 11:01:45 2017 +0200
> > # Node ID a450dc69ef95073af4d4ece34e251ed0bb5b808f
> > # Parent  537de0b14030868e3e850ae388b08f88cabc88e8
> > # EXP-Topic b2.bookmarks
> > # Available At https://bitbucket.org/octobus/mercurial-devel/
> > #              hg pull https://bitbucket.org/octobus/mercurial-deve
> > l/ -r a450dc69ef95
> > pull: store binary node in pullop.remotebookmarks
> > 
> > The internal representation of bookmark value is binary. The fact
> > we stored
> > 'hex' was an implementation detail from using pushkey.
> > 
> > We move the values in 'pullop.remotebookmarks' to binary before
> > adding a way to
> > exchange bookmarks not based on pushkey.
> 
> The overall approach seems reasonable and works. But the wire
> protocol is one of those things that you need to support forever. So
> it is important that any changes get strict scrutiny.
> 
> General questions/feedback:
> 
> * The series does the simplest thing and basically copies the old
> bookmarks logic to bundle2. What are your thoughts on changing the
> structure a bit? e.g. instead of separate check and set bundle parts,
> what about a single part that records the old state of bookmarks?
> We'd need to send this first to facilitate aborting early on
> consistency mismatch, of course. I'm not claiming one approach is
> better: I'm just throwing out an alternative to consider.

Actually, the current logic (with pushkey) is already using this part
for check and set.

One of the goals of this series was to split the two parts:
    
* The check part is rarely used. It is only necessary for (unforced)
bookmark push.
* Every other usage (pull, bundle, forced push, etc) just need the
other part. So keeping them separated is an improvement.

Also, I'm afraid that receiving the bookmarks part before the changeset
part would make the bookmarks part handling more complex. We would have
to keep the data around 
until the changeset it refers actually shows up.

> 
> * There are a number of places that could benefit from early return
> control flow.
> 
> * I believe after this series we now have both a "bookmarks" query
> string argument *and* a "bookmarks" entry in the listkeys value for
> bundle requests. Is this redundancy needed? Could you please document
> any new arguments in wireproto.txt.

Yes, we would have both but we would use one or the other, never both
at the same time. The redundancy is needed for handling pre-Mercurial
4.4 clients or servers as we could not use the new bookmarks part. We
will update the wireproto.txt documentation.

> 
> * The config option related to pushkey hooks firing is seemingly very
> low-level. I'm tempted to omit the option and instead establish a BC
> schedule to drop the old behavior (assuming we don't want to support
> it forever).

Just dropping it seems dangerous for our users. What about:

* mark the option as 'advanced' (not documented unless -v is used).

* change the default to False in a couple of versions.

> 
> * While we're here, a feature I've long wanted to implement is `hg
> pull --mirror`. Please spend a few seconds contemplating if that
> would require any wire protocol changes to support for bookmarks
> exchange. If so, we may want to implement it now so we don't have to
> implement more "protocol negotiation" later.

We don't need any protocol update for this feature. We can do it with
listkey today and we will be able to do it with the bookmarks part
tomorrow.

> 
> * Do we have sufficient test coverage around force pushing for both
> the old and new style of bookmarks exchange? I could see that as one
> of those things that easily regresses unless we're actively testing
> both variations.

Yes, we made sure to run all existing exchange tests with both versions
(pushkey and bookmarks part). So we should be as covered as before.

> 
> * Landing wire protocol changes just before the freeze is a bit
> scary. I know this is a major bug and I want to see it fixed as well.
> But the schedule is concerning. I'd feel much more comfortable if
> this were behind a config knob until 4.5.

I'm not sure to understand what you mean? Do want us to send a V2 with
a config knob that would be included in the 4.4 release? Or do we wait
for the 4.5 cycle to start to send a V2? Giving the current timing,
waiting for 4.5 is preferred on our side.

>  
> > diff --git a/mercurial/commands.py b/mercurial/commands.py
> > --- a/mercurial/commands.py
> > +++ b/mercurial/commands.py
> > @@ -3911,12 +3911,13 @@
> >              # not ending up with the name of the bookmark because
> > of a race
> >              # condition on the server. (See issue 4689 for
> > details)
> >              remotebookmarks = other.listkeys('bookmarks')
> > +            remotebookmarks =
> > bookmarks.unhexlifybookmarks(remotebookmarks)
> >              pullopargs['remotebookmarks'] = remotebookmarks
> >              for b in opts['bookmark']:
> >                  b = repo._bookmarks.expandname(b)
> >                  if b not in remotebookmarks:
> >                      raise error.Abort(_('remote bookmark %s not
> > found!') % b)
> > -                revs.append(remotebookmarks[b])
> > +                revs.append(hex(remotebookmarks[b]))
> > 
> >          if revs:
> >              try:
> > diff --git a/mercurial/exchange.py b/mercurial/exchange.py
> > --- a/mercurial/exchange.py
> > +++ b/mercurial/exchange.py
> > @@ -1348,7 +1348,8 @@
> >          # all known bundle2 servers now support listkeys, but lets
> > be nice with
> >          # new implementation.
> >          return
> > -    pullop.remotebookmarks = pullop.remote.listkeys('bookmarks')
> > +    books = pullop.remote.listkeys('bookmarks')
> > +    pullop.remotebookmarks = bookmod.unhexlifybookmarks(books)
> > 
> > 
> >  @pulldiscovery('changegroup')
> > @@ -1459,7 +1460,7 @@
> >      # processing bookmark update
> >      for namespace, value in op.records['listkeys']:
> >          if namespace == 'bookmarks':
> > -            pullop.remotebookmarks = value
> > +            pullop.remotebookmarks =
> > bookmod.unhexlifybookmarks(value)
> > 
> >      # bookmark data were either already there or pulled in the
> > bundle
> >      if pullop.remotebookmarks is not None:
> > @@ -1552,7 +1553,6 @@
> >      pullop.stepsdone.add('bookmarks')
> >      repo = pullop.repo
> >      remotebookmarks = pullop.remotebookmarks
> > -    remotebookmarks = bookmod.unhexlifybookmarks(remotebookmarks)
> >      bookmod.updatefromremote(repo.ui, repo, remotebookmarks,
> >                               pullop.remote.url(),
> >                               pullop.gettransaction,
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> > 
> 
>

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -3911,12 +3911,13 @@ 
             # not ending up with the name of the bookmark because of a race
             # condition on the server. (See issue 4689 for details)
             remotebookmarks = other.listkeys('bookmarks')
+            remotebookmarks = bookmarks.unhexlifybookmarks(remotebookmarks)
             pullopargs['remotebookmarks'] = remotebookmarks
             for b in opts['bookmark']:
                 b = repo._bookmarks.expandname(b)
                 if b not in remotebookmarks:
                     raise error.Abort(_('remote bookmark %s not found!') % b)
-                revs.append(remotebookmarks[b])
+                revs.append(hex(remotebookmarks[b]))
 
         if revs:
             try:
diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -1348,7 +1348,8 @@ 
         # all known bundle2 servers now support listkeys, but lets be nice with
         # new implementation.
         return
-    pullop.remotebookmarks = pullop.remote.listkeys('bookmarks')
+    books = pullop.remote.listkeys('bookmarks')
+    pullop.remotebookmarks = bookmod.unhexlifybookmarks(books)
 
 
 @pulldiscovery('changegroup')
@@ -1459,7 +1460,7 @@ 
     # processing bookmark update
     for namespace, value in op.records['listkeys']:
         if namespace == 'bookmarks':
-            pullop.remotebookmarks = value
+            pullop.remotebookmarks = bookmod.unhexlifybookmarks(value)
 
     # bookmark data were either already there or pulled in the bundle
     if pullop.remotebookmarks is not None:
@@ -1552,7 +1553,6 @@ 
     pullop.stepsdone.add('bookmarks')
     repo = pullop.repo
     remotebookmarks = pullop.remotebookmarks
-    remotebookmarks = bookmod.unhexlifybookmarks(remotebookmarks)
     bookmod.updatefromremote(repo.ui, repo, remotebookmarks,
                              pullop.remote.url(),
                              pullop.gettransaction,