Patchwork [8,of,9,RFC] wireproto: introduce listkeys2 command

login
register
mail settings
Submitter Gregory Szorc
Date Aug. 14, 2016, 9:10 p.m.
Message ID <d2870bcbc43041909e9f.1471209007@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/16287/
State Changes Requested
Headers show

Comments

Gregory Szorc - Aug. 14, 2016, 9:10 p.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1471208237 25200
#      Sun Aug 14 13:57:17 2016 -0700
# Node ID d2870bcbc43041909e9f637b294cb889f7ed4933
# Parent  eb2bc1ac7869ad255965d16004524a95cea83c9d
wireproto: introduce listkeys2 command

The command behaves exactly like "listkeys" except it uses a more
efficient and more robust binary encoding mechanism.
Mike Hommey - Aug. 14, 2016, 10:12 p.m.
On Sun, Aug 14, 2016 at 02:10:07PM -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1471208237 25200
> #      Sun Aug 14 13:57:17 2016 -0700
> # Node ID d2870bcbc43041909e9f637b294cb889f7ed4933
> # Parent  eb2bc1ac7869ad255965d16004524a95cea83c9d
> wireproto: introduce listkeys2 command
> 
> The command behaves exactly like "listkeys" except it uses a more
> efficient and more robust binary encoding mechanism.

Nowhere in the patch queue I see mentioned why you want this. Not saying
that this shouldn't be done, but it's really not clear what the expected
benefit is of all this refactoring and this new command.

Mike
Gregory Szorc - Aug. 14, 2016, 11:59 p.m.
On Sun, Aug 14, 2016 at 3:12 PM, Mike Hommey <mh@glandium.org> wrote:

> On Sun, Aug 14, 2016 at 02:10:07PM -0700, Gregory Szorc wrote:
> > # HG changeset patch
> > # User Gregory Szorc <gregory.szorc@gmail.com>
> > # Date 1471208237 25200
> > #      Sun Aug 14 13:57:17 2016 -0700
> > # Node ID d2870bcbc43041909e9f637b294cb889f7ed4933
> > # Parent  eb2bc1ac7869ad255965d16004524a95cea83c9d
> > wireproto: introduce listkeys2 command
> >
> > The command behaves exactly like "listkeys" except it uses a more
> > efficient and more robust binary encoding mechanism.
>
> Nowhere in the patch queue I see mentioned why you want this. Not saying
> that this shouldn't be done, but it's really not clear what the expected
> benefit is of all this refactoring and this new command.


I said it concisely in the commit message you just quoted: the wire
encoding is smaller and is able to represent all binary values. I offer
more detail at
https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-August/087243.html.
Mike Hommey - Aug. 15, 2016, 1:04 a.m.
On Sun, Aug 14, 2016 at 04:59:50PM -0700, Gregory Szorc wrote:
> On Sun, Aug 14, 2016 at 3:12 PM, Mike Hommey <mh@glandium.org> wrote:
> 
> > On Sun, Aug 14, 2016 at 02:10:07PM -0700, Gregory Szorc wrote:
> > > # HG changeset patch
> > > # User Gregory Szorc <gregory.szorc@gmail.com>
> > > # Date 1471208237 25200
> > > #      Sun Aug 14 13:57:17 2016 -0700
> > > # Node ID d2870bcbc43041909e9f637b294cb889f7ed4933
> > > # Parent  eb2bc1ac7869ad255965d16004524a95cea83c9d
> > > wireproto: introduce listkeys2 command
> > >
> > > The command behaves exactly like "listkeys" except it uses a more
> > > efficient and more robust binary encoding mechanism.
> >
> > Nowhere in the patch queue I see mentioned why you want this. Not saying
> > that this shouldn't be done, but it's really not clear what the expected
> > benefit is of all this refactoring and this new command.
> 
> 
> I said it concisely in the commit message you just quoted: the wire
> encoding is smaller and is able to represent all binary values. I offer
> more detail at
> https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-August/087243.html.

... and a large part of that message should be in this commit message.

Mike
Augie Fackler - Aug. 16, 2016, 1:58 p.m.
On Mon, Aug 15, 2016 at 10:04:34AM +0900, Mike Hommey wrote:
> On Sun, Aug 14, 2016 at 04:59:50PM -0700, Gregory Szorc wrote:
> > On Sun, Aug 14, 2016 at 3:12 PM, Mike Hommey <mh@glandium.org> wrote:
> >
> > > On Sun, Aug 14, 2016 at 02:10:07PM -0700, Gregory Szorc wrote:
> > > > # HG changeset patch
> > > > # User Gregory Szorc <gregory.szorc@gmail.com>
> > > > # Date 1471208237 25200
> > > > #      Sun Aug 14 13:57:17 2016 -0700
> > > > # Node ID d2870bcbc43041909e9f637b294cb889f7ed4933
> > > > # Parent  eb2bc1ac7869ad255965d16004524a95cea83c9d
> > > > wireproto: introduce listkeys2 command
> > > >
> > > > The command behaves exactly like "listkeys" except it uses a more
> > > > efficient and more robust binary encoding mechanism.
> > >
> > > Nowhere in the patch queue I see mentioned why you want this. Not saying
> > > that this shouldn't be done, but it's really not clear what the expected
> > > benefit is of all this refactoring and this new command.
> >
> >
> > I said it concisely in the commit message you just quoted: the wire
> > encoding is smaller and is able to represent all binary values. I offer
> > more detail at
> > https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-August/087243.html.
>
> ... and a large part of that message should be in this commit message.

+1. I think overall I like the series.

>
> Mike
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Sean Farley - Aug. 16, 2016, 9:03 p.m.
Augie Fackler <raf@durin42.com> writes:

> On Mon, Aug 15, 2016 at 10:04:34AM +0900, Mike Hommey wrote:
>> On Sun, Aug 14, 2016 at 04:59:50PM -0700, Gregory Szorc wrote:
>> > On Sun, Aug 14, 2016 at 3:12 PM, Mike Hommey <mh@glandium.org> wrote:
>> >
>> > > On Sun, Aug 14, 2016 at 02:10:07PM -0700, Gregory Szorc wrote:
>> > > > # HG changeset patch
>> > > > # User Gregory Szorc <gregory.szorc@gmail.com>
>> > > > # Date 1471208237 25200
>> > > > #      Sun Aug 14 13:57:17 2016 -0700
>> > > > # Node ID d2870bcbc43041909e9f637b294cb889f7ed4933
>> > > > # Parent  eb2bc1ac7869ad255965d16004524a95cea83c9d
>> > > > wireproto: introduce listkeys2 command
>> > > >
>> > > > The command behaves exactly like "listkeys" except it uses a more
>> > > > efficient and more robust binary encoding mechanism.
>> > >
>> > > Nowhere in the patch queue I see mentioned why you want this. Not saying
>> > > that this shouldn't be done, but it's really not clear what the expected
>> > > benefit is of all this refactoring and this new command.
>> >
>> >
>> > I said it concisely in the commit message you just quoted: the wire
>> > encoding is smaller and is able to represent all binary values. I offer
>> > more detail at
>> > https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-August/087243.html.
>>
>> ... and a large part of that message should be in this commit message.
>
> +1. I think overall I like the series.

Maybe a better name, though? e.g. listkeysraw or something

Also, I think this reworks listkeys to use this implementation, is that
correct?

Patch

diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py
--- a/mercurial/wireproto.py
+++ b/mercurial/wireproto.py
@@ -358,16 +358,27 @@  class wirepeer(peer.peerrepository):
         f = future()
         self.ui.debug('preparing listkeys for "%s"\n' % namespace)
         yield {'namespace': encoding.fromlocal(namespace)}, f
         d = f.value
         self.ui.debug('received listkey for "%s": %i bytes\n'
                       % (namespace, len(d)))
         yield pushkeymod.decodekeys(d)
 
+    @batchable
+    def listkeys2(self, namespace):
+        self.requirecap('listkeys2', 'binary pushkey retrieval')
+        f = future()
+        self.ui.debug('preparing listkeys2 for "%s"\n' % namespace)
+        yield {'namespace': encoding.fromlocal(namespace)}, f
+        d = f.value
+        self.ui.debug('received listkeys2 for "%s": %i bytes\n'
+                      % (namespace, len(d)))
+        yield pushkeymod.decodekeysraw(d)
+
     def stream_out(self):
         return self._callstream('stream_out')
 
     def changegroup(self, nodes, kind):
         n = encodelist(nodes)
         f = self._callcompressable("changegroup", roots=n)
         return changegroupmod.cg1unpacker(f, 'UN')
 
@@ -681,17 +692,17 @@  def clonebundles(repo, proto):
 
     Extensions may wrap this command to filter or dynamically emit data
     depending on the request. e.g. you could advertise URLs for the closest
     data center given the client's IP address.
     """
     return repo.opener.tryread('clonebundles.manifest')
 
 wireprotocaps = ['lookup', 'changegroupsubset', 'branchmap', 'pushkey',
-                 'known', 'getbundle', 'unbundlehash', 'batch']
+                 'known', 'getbundle', 'unbundlehash', 'batch', 'listkeys2']
 
 def _capabilities(repo, proto):
     """return a list of capabilities for a repo
 
     This function exists to allow extensions to easily wrap capabilities
     computation
 
     - returns a lists: easy to alter
@@ -790,16 +801,21 @@  def hello(repo, proto):
     '''
     return "capabilities: %s\n" % (capabilities(repo, proto))
 
 @wireprotocommand('listkeys', 'namespace')
 def listkeys(repo, proto, namespace):
     d = repo.listkeys(encoding.tolocal(namespace)).items()
     return pushkeymod.encodekeys(d)
 
+@wireprotocommand('listkeys2', 'namespace')
+def listkeys2(repo, proto, namespace):
+    d = repo.listkeys(encoding.tolocal(namespace), raw=True).items()
+    return pushkeymod.encodekeysraw(d)
+
 @wireprotocommand('lookup', 'key')
 def lookup(repo, proto, key):
     try:
         k = encoding.tolocal(key)
         c = repo[k]
         r = c.hex()
         success = 1
     except Exception as inst:
diff --git a/tests/test-hgweb-commands.t b/tests/test-hgweb-commands.t
--- a/tests/test-hgweb-commands.t
+++ b/tests/test-hgweb-commands.t
@@ -1898,17 +1898,17 @@  raw graph
   
   
 
 capabilities
 
   $ get-with-headers.py 127.0.0.1:$HGPORT '?cmd=capabilities'; echo
   200 Script output follows
   
-  lookup changegroupsubset branchmap pushkey known getbundle unbundlehash batch bundle2=HG20%0Achangegroup%3D01%2C02%0Adigests%3Dmd5%2Csha1%2Csha512%0Aerror%3Dabort%2Cunsupportedcontent%2Cpushraced%2Cpushkey%0Ahgtagsfnodes%0Alistkeys%0Apushkey%0Aremote-changegroup%3Dhttp%2Chttps unbundle=HG10GZ,HG10BZ,HG10UN httpheader=1024
+  lookup changegroupsubset branchmap pushkey known getbundle unbundlehash batch listkeys2 bundle2=HG20%0Achangegroup%3D01%2C02%0Adigests%3Dmd5%2Csha1%2Csha512%0Aerror%3Dabort%2Cunsupportedcontent%2Cpushraced%2Cpushkey%0Ahgtagsfnodes%0Alistkeys%0Apushkey%0Aremote-changegroup%3Dhttp%2Chttps unbundle=HG10GZ,HG10BZ,HG10UN httpheader=1024
 
 heads
 
   $ get-with-headers.py 127.0.0.1:$HGPORT '?cmd=heads'
   200 Script output follows
   
   cad8025a2e87f88c06259790adfa15acb4080123
 
@@ -2141,16 +2141,17 @@  capabilities
   lookup
   changegroupsubset
   branchmap
   pushkey
   known
   getbundle
   unbundlehash
   batch
+  listkeys2
   stream-preferred
   streamreqs=generaldelta,revlogv1
   bundle2=HG20%0Achangegroup%3D01%2C02%0Adigests%3Dmd5%2Csha1%2Csha512%0Aerror%3Dabort%2Cunsupportedcontent%2Cpushraced%2Cpushkey%0Ahgtagsfnodes%0Alistkeys%0Apushkey%0Aremote-changegroup%3Dhttp%2Chttps
   unbundle=HG10GZ,HG10BZ,HG10UN
   httpheader=1024
 
 heads
 
diff --git a/tests/test-ssh-bundle1.t b/tests/test-ssh-bundle1.t
--- a/tests/test-ssh-bundle1.t
+++ b/tests/test-ssh-bundle1.t
@@ -457,18 +457,18 @@  stderr from remote commands should be pr
 
 debug output
 
   $ hg pull --debug ssh://user@dummy/remote
   pulling from ssh://user@dummy/remote
   running python ".*/dummyssh" user@dummy ('|")hg -R remote serve --stdio('|") (re)
   sending hello command
   sending between command
-  remote: 371
-  remote: capabilities: lookup changegroupsubset branchmap pushkey known getbundle unbundlehash batch streamreqs=generaldelta,revlogv1 bundle2=HG20%0Achangegroup%3D01%2C02%0Adigests%3Dmd5%2Csha1%2Csha512%0Aerror%3Dabort%2Cunsupportedcontent%2Cpushraced%2Cpushkey%0Ahgtagsfnodes%0Alistkeys%0Apushkey%0Aremote-changegroup%3Dhttp%2Chttps unbundle=HG10GZ,HG10BZ,HG10UN httpheader=1024
+  remote: 381
+  remote: capabilities: lookup changegroupsubset branchmap pushkey known getbundle unbundlehash batch listkeys2 streamreqs=generaldelta,revlogv1 bundle2=HG20%0Achangegroup%3D01%2C02%0Adigests%3Dmd5%2Csha1%2Csha512%0Aerror%3Dabort%2Cunsupportedcontent%2Cpushraced%2Cpushkey%0Ahgtagsfnodes%0Alistkeys%0Apushkey%0Aremote-changegroup%3Dhttp%2Chttps unbundle=HG10GZ,HG10BZ,HG10UN httpheader=1024
   remote: 1
   preparing listkeys for "bookmarks"
   sending listkeys command
   received listkey for "bookmarks": 45 bytes
   query 1; heads
   sending batch command
   searching for changes
   all remote heads known locally
diff --git a/tests/test-ssh.t b/tests/test-ssh.t
--- a/tests/test-ssh.t
+++ b/tests/test-ssh.t
@@ -449,18 +449,18 @@  stderr from remote commands should be pr
 
 debug output
 
   $ hg pull --debug ssh://user@dummy/remote
   pulling from ssh://user@dummy/remote
   running python ".*/dummyssh" user@dummy ('|")hg -R remote serve --stdio('|") (re)
   sending hello command
   sending between command
-  remote: 371
-  remote: capabilities: lookup changegroupsubset branchmap pushkey known getbundle unbundlehash batch streamreqs=generaldelta,revlogv1 bundle2=HG20%0Achangegroup%3D01%2C02%0Adigests%3Dmd5%2Csha1%2Csha512%0Aerror%3Dabort%2Cunsupportedcontent%2Cpushraced%2Cpushkey%0Ahgtagsfnodes%0Alistkeys%0Apushkey%0Aremote-changegroup%3Dhttp%2Chttps unbundle=HG10GZ,HG10BZ,HG10UN httpheader=1024
+  remote: 381
+  remote: capabilities: lookup changegroupsubset branchmap pushkey known getbundle unbundlehash batch listkeys2 streamreqs=generaldelta,revlogv1 bundle2=HG20%0Achangegroup%3D01%2C02%0Adigests%3Dmd5%2Csha1%2Csha512%0Aerror%3Dabort%2Cunsupportedcontent%2Cpushraced%2Cpushkey%0Ahgtagsfnodes%0Alistkeys%0Apushkey%0Aremote-changegroup%3Dhttp%2Chttps unbundle=HG10GZ,HG10BZ,HG10UN httpheader=1024
   remote: 1
   query 1; heads
   sending batch command
   searching for changes
   all remote heads known locally
   no changes found
   sending getbundle command
   bundle2-input-bundle: with-transaction