Patchwork D1962: setdiscovery: don't call "heads" wire command when heads specified

login
register
mail settings
Submitter phabricator
Date Feb. 1, 2018, 7:52 p.m.
Message ID <differential-rev-PHID-DREV-hw6fnr2ul2ikszdii7uw-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/27105/
State Superseded
Headers show

Comments

phabricator - Feb. 1, 2018, 7:52 p.m.
martinvonz created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Our custom server has too many heads to announce (one per code review,
  plus a public head), but it still lets the user request one of them by
  doing
  
    hg pull -r <some expression>
  
  After the client has resolved the expression to a set of nodeids by
  calling the "lookup" wire command, it will start the discovery
  phase. Before this patch, that doesn't take the requested heads into
  account and unconditionally calls the server's "heads" command to find
  all its heads. One consequence of that the "all remote heads known
  locally" case triggers if the client already had the public head and
  the user will see a "no changes found" message that's unrelated to the
  head they requested. That message confused me for a while. More
  imporantly, it also means that pullop.cgresult incorrectly (given our
  arguably misbehaving server) gets set to 0 (no changesets added),
  which confused some of our extensions.
  
  This patch makes it so the client skips the "heads" command if the
  user requested specific revisions.
  
  Since the "heads" command is normally batched with the first "known"
  command and calculating the list of heads is probably cheap, I don't
  expect much improvement in speed from this.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/discovery.py
  mercurial/setdiscovery.py

CHANGE DETAILS




To: martinvonz, #hg-reviewers
Cc: mercurial-devel
phabricator - Feb. 1, 2018, 8:48 p.m.
indygreg accepted this revision.
indygreg added a comment.
This revision is now accepted and ready to land.


  I initially thought this would have test fallout. But since we're using a batch command to issue the `heads` wire protocol command, it won't show up in HTTP logs. We'd only catch it in verbose/debug logging, which we don't do a lot of. So no test fallout is expected.
  
  FWIW, while computing DAG heads is probably cheap, sending them over the wire may not be. 41 bytes per head can add up pretty quickly and turn what would be a <1kb response into several kb.

REPOSITORY
  rHG Mercurial

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

To: martinvonz, #hg-reviewers, indygreg
Cc: indygreg, mercurial-devel

Patch

diff --git a/mercurial/setdiscovery.py b/mercurial/setdiscovery.py
--- a/mercurial/setdiscovery.py
+++ b/mercurial/setdiscovery.py
@@ -130,7 +130,7 @@ 
         sample = set(random.sample(sample, desiredlen))
     return sample
 
-def findcommonheads(ui, local, remote,
+def findcommonheads(ui, local, remote, heads=None,
                     initialsamplesize=100,
                     fullsamplesize=200,
                     abortwhenunrelated=True,
@@ -155,11 +155,15 @@ 
     sample = _limitsample(ownheads, initialsamplesize)
     # indices between sample and externalized version must match
     sample = list(sample)
-    batch = remote.iterbatch()
-    batch.heads()
-    batch.known(dag.externalizeall(sample))
-    batch.submit()
-    srvheadhashes, yesno = batch.results()
+    if heads:
+        srvheadhashes = heads
+        yesno = remote.known(dag.externalizeall(sample))
+    else:
+        batch = remote.iterbatch()
+        batch.heads()
+        batch.known(dag.externalizeall(sample))
+        batch.submit()
+        srvheadhashes, yesno = batch.results()
 
     if cl.tip() == nullid:
         if srvheadhashes != [nullid]:
diff --git a/mercurial/discovery.py b/mercurial/discovery.py
--- a/mercurial/discovery.py
+++ b/mercurial/discovery.py
@@ -62,7 +62,7 @@ 
         if allknown:
             return (heads, False, heads)
 
-    res = setdiscovery.findcommonheads(repo.ui, repo, remote,
+    res = setdiscovery.findcommonheads(repo.ui, repo, remote, heads,
                                        abortwhenunrelated=not force,
                                        ancestorsof=ancestorsof)
     common, anyinc, srvheads = res