Patchwork D6851: narrow: don't hexify paths and double-hexify known nodes on wire (BC)

login
register
mail settings
Submitter phabricator
Date Sept. 14, 2019, 5:11 p.m.
Message ID <differential-rev-PHID-DREV-r4iaixkupvwnzx57lmzq-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/41670/
State Superseded
Headers show

Comments

phabricator - Sept. 14, 2019, 5:11 p.m.
martinvonz created this revision.
Herald added a reviewer: durin42.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  It isn't obvious, but wireprototypes.encodelist() is meant only for
  binary nodeids. So when we used it for encoding hex nodeids and paths,
  the encoded result was surprising and hard to read.
  
  This patch changes the encoding to make the list of paths a
  null-separated list and the list of common nodes to be a
  encodelist()-encoded list of binary nodeids (so the result is just
  singly-hexified nodeids).
  
  This is clearly a breaking change, but the feature is experimental and
  we're not aware of anyone running a server using this command yet.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6851

AFFECTED FILES
  hgext/narrow/narrowcommands.py
  hgext/narrow/narrowwirepeer.py
  relnotes/next

CHANGE DETAILS




To: martinvonz, durin42, #hg-reviewers
Cc: mercurial-devel
phabricator - Sept. 16, 2019, 4:49 a.m.
martinvonz added a comment.


  Hmm, I just noticed that we separate these paths by commas (not nulls) in other places, so I'll make this consistent with that. I guess that means we don't support paths containing commas for includes or excludes, but fixing that is a separate problem.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6851/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6851

To: martinvonz, durin42, #hg-reviewers
Cc: mercurial-devel
phabricator - Sept. 16, 2019, 5:05 p.m.
pulkit added a comment.
pulkit added subscribers: idlsoft, pulkit.


  > This is clearly a breaking change, but the feature is experimental and
  > we're not aware of anyone running a server using this command yet.
  
  @idlsoft and their company does use narrow extension. @idlsoft can you upgrade server and client at the same time?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6851/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6851

To: martinvonz, durin42, #hg-reviewers
Cc: pulkit, idlsoft, mercurial-devel
phabricator - Sept. 16, 2019, 5:31 p.m.
idlsoft added a comment.


  > @idlsoft and their company does use narrow extension. @idlsoft can you upgrade server and client at the same time?
  
  I did that a little while ago to move to 5.0. It was not fun. It's server, teamcity, clients, docker images.
  What operations does this affect? Regular `push/pull/clone` or only `tracked --add-include`?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6851/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6851

To: martinvonz, durin42, #hg-reviewers
Cc: pulkit, idlsoft, mercurial-devel
phabricator - Sept. 16, 2019, 5:39 p.m.
martinvonz added a comment.


  In D6851#100666 <https://phab.mercurial-scm.org/D6851#100666>, @idlsoft wrote:
  
  >> @idlsoft and their company does use narrow extension. @idlsoft can you upgrade server and client at the same time?
  >
  > I did that a little while ago to move to 5.0. It was not fun. It's server, teamcity, clients, docker images.
  > What operations does this affect? Regular `push/pull/clone` or only `tracked --add-include`?
  
  Just `tracked --add-include`. A workaround to simplify the upgrade would be to change `wireprototypes.SUPPORTED_ELLIPSESCAP` to be `(ELLIPSESCAP1, )` on the server from now until all clients have upgraded. But that may still be annoying and error-prone for you to deal with. @pulkit, I suppose we should just add a `exp-narrow-2` capability to deal with this? It doesn't seem fair to make @idlsoft deal with it.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6851/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6851

To: martinvonz, durin42, #hg-reviewers
Cc: pulkit, idlsoft, mercurial-devel
phabricator - Sept. 16, 2019, 5:47 p.m.
pulkit added a comment.


  In D6851#100667 <https://phab.mercurial-scm.org/D6851#100667>, @martinvonz wrote:
  
  > In D6851#100666 <https://phab.mercurial-scm.org/D6851#100666>, @idlsoft wrote:
  >
  >>> @idlsoft and their company does use narrow extension. @idlsoft can you upgrade server and client at the same time?
  >>
  >> I did that a little while ago to move to 5.0. It was not fun. It's server, teamcity, clients, docker images.
  >> What operations does this affect? Regular `push/pull/clone` or only `tracked --add-include`?
  >
  > Just `tracked --add-include`. A workaround to simplify the upgrade would be to change `wireprototypes.SUPPORTED_ELLIPSESCAP` to be `(ELLIPSESCAP1, )` on the server from now until all clients have upgraded. But that may still be annoying and error-prone for you to deal with. @pulkit, I suppose we should just add a `exp-narrow-2` capability to deal with this? It doesn't seem fair to make @idlsoft deal with it.
  
  Sounds like a good idea!

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6851/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6851

To: martinvonz, durin42, #hg-reviewers
Cc: pulkit, idlsoft, mercurial-devel
phabricator - Sept. 16, 2019, 6:03 p.m.
idlsoft added a comment.


  >> Just `tracked --add-include`. A workaround to simplify the upgrade would be to change `wireprototypes.SUPPORTED_ELLIPSESCAP` to be `(ELLIPSESCAP1, )` on the server from now until all clients have upgraded. But that may still be annoying and error-prone for you to deal with. @pulkit, I suppose we should just add a `exp-narrow-2` capability to deal with this? It doesn't seem fair to make @idlsoft deal with it.
  >
  > Sounds like a good idea!
  
  If it's just `tracked --add-include` then it's not a big deal, it won't disrupt regular flow.
  If backward compatibility doesn't complicate the code - great, if not - don't worry about it.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6851/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6851

To: martinvonz, durin42, #hg-reviewers
Cc: pulkit, idlsoft, mercurial-devel
phabricator - Sept. 16, 2019, 6:14 p.m.
martinvonz added a comment.


  In D6851#100690 <https://phab.mercurial-scm.org/D6851#100690>, @idlsoft wrote:
  
  >>> Just `tracked --add-include`. A workaround to simplify the upgrade would be to change `wireprototypes.SUPPORTED_ELLIPSESCAP` to be `(ELLIPSESCAP1, )` on the server from now until all clients have upgraded. But that may still be annoying and error-prone for you to deal with. @pulkit, I suppose we should just add a `exp-narrow-2` capability to deal with this? It doesn't seem fair to make @idlsoft deal with it.
  >>
  >> Sounds like a good idea!
  >
  > If it's just `tracked --add-include` then it's not a big deal, it won't disrupt regular flow.
  > If backward compatibility doesn't complicate the code - great, if not - don't worry about it.
  
  It does seem to be more work than I hoped at first, so it would be very much appreciated if you can live with `hg tracked --add-include` (and `--remove-exclude`) not working while you transition server and clients.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6851/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6851

To: martinvonz, durin42, #hg-reviewers
Cc: pulkit, idlsoft, mercurial-devel
phabricator - Sept. 17, 2019, 5:45 p.m.
This revision is now accepted and ready to land.
pulkit added a comment.
pulkit accepted this revision.


  In D6851#100690 <https://phab.mercurial-scm.org/D6851#100690>, @idlsoft wrote:
  
  >>> Just `tracked --add-include`. A workaround to simplify the upgrade would be to change `wireprototypes.SUPPORTED_ELLIPSESCAP` to be `(ELLIPSESCAP1, )` on the server from now until all clients have upgraded. But that may still be annoying and error-prone for you to deal with. @pulkit, I suppose we should just add a `exp-narrow-2` capability to deal with this? It doesn't seem fair to make @idlsoft deal with it.
  >>
  >> Sounds like a good idea!
  >
  > If it's just `tracked --add-include` then it's not a big deal, it won't disrupt regular flow.
  > If backward compatibility doesn't complicate the code - great, if not - don't worry about it.
  
  Great, thanks!

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6851/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6851

To: martinvonz, durin42, #hg-reviewers, pulkit
Cc: pulkit, idlsoft, mercurial-devel

Patch

diff --git a/relnotes/next b/relnotes/next
--- a/relnotes/next
+++ b/relnotes/next
@@ -15,5 +15,9 @@ 
    you can override it by setting `$HGTEST_SHELL` or by passing it to
    `run-tests.py --shell <shell>`.
 
+ * The narrow extension's wire protocol changed. If you're using it,
+   you'll need to make sure to upgrade server and client at the same
+   time.
+
 == Internal API Changes ==
 
diff --git a/hgext/narrow/narrowwirepeer.py b/hgext/narrow/narrowwirepeer.py
--- a/hgext/narrow/narrowwirepeer.py
+++ b/hgext/narrow/narrowwirepeer.py
@@ -13,7 +13,6 @@ 
     extensions,
     hg,
     narrowspec,
-    node as nodemod,
     pycompat,
     wireprototypes,
     wireprotov1peer,
@@ -61,10 +60,13 @@ 
 
     preferuncompressed = False
     try:
-        oldincludes = wireprototypes.decodelist(oldincludes)
-        newincludes = wireprototypes.decodelist(newincludes)
-        oldexcludes = wireprototypes.decodelist(oldexcludes)
-        newexcludes = wireprototypes.decodelist(newexcludes)
+        def splitpaths(data):
+            # work around ''.split('\0') => ['']
+            return data.split(b'\0') if data else []
+        oldincludes = splitpaths(oldincludes)
+        newincludes = splitpaths(newincludes)
+        oldexcludes = splitpaths(oldexcludes)
+        newexcludes = splitpaths(newexcludes)
         # validate the patterns
         narrowspec.validatepatterns(set(oldincludes))
         narrowspec.validatepatterns(set(newincludes))
@@ -73,7 +75,6 @@ 
 
         common = wireprototypes.decodelist(commonheads)
         known = wireprototypes.decodelist(known)
-        known = {nodemod.bin(n) for n in known}
         if ellipses == '0':
             ellipses = False
         else:
@@ -106,10 +107,12 @@ 
                                     prefer_uncompressed=preferuncompressed)
 
 def peernarrowwiden(remote, **kwargs):
-    for ch in (r'oldincludes', r'newincludes', r'oldexcludes', r'newexcludes',
-               r'commonheads', r'known'):
+    for ch in (r'commonheads', r'known'):
         kwargs[ch] = wireprototypes.encodelist(kwargs[ch])
 
+    for ch in (r'oldincludes', r'newincludes', r'oldexcludes', r'newexcludes'):
+        kwargs[ch] = b'\0'.join(kwargs[ch])
+
     kwargs[r'ellipses'] = '%i' % bool(kwargs[r'ellipses'])
     f = remote._callcompressable('narrow_widen', **kwargs)
     return bundle2.getunbundler(remote.ui, f)
diff --git a/hgext/narrow/narrowcommands.py b/hgext/narrow/narrowcommands.py
--- a/hgext/narrow/narrowcommands.py
+++ b/hgext/narrow/narrowcommands.py
@@ -293,7 +293,7 @@ 
         else:
             known = []
             if ellipsesremote:
-                known = [node.hex(ctx.node()) for ctx in
+                known = [ctx.node() for ctx in
                          repo.set('::%ln', common)
                          if ctx.node() != node.nullid]
             with remote.commandexecutor() as e: