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
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
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.
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
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
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