This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1962

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

Authored by martinvonz on Feb 1 2018, 2:52 PM.

Details

Reviewers
indygreg
Group Reviewers
hg-reviewers
Commits
rHG5cfdf6137af8: setdiscovery: don't call "heads" wire command when heads specified
rHG98b746a1ebe3: setdiscovery: don't call "heads" wire command when heads specified
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.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

martinvonz created this revision.Feb 1 2018, 2:52 PM
indygreg accepted this revision.Feb 1 2018, 3:48 PM
indygreg added a subscriber: indygreg.

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.

This revision is now accepted and ready to land.Feb 1 2018, 3:48 PM
Closed by commit rHG98b746a1ebe3: setdiscovery: don't call "heads" wire command when heads specified (authored by martinvonz). · Explain WhyFeb 1 2018, 3:48 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/discovery.py (2 lines)
Mmercurial/setdiscovery.py (16 lines)

Diff 5036

mercurial/discovery.py

knownnode = repo.changelog.hasnode # no nodemap until it is filtered knownnode = repo.changelog.hasnode # no nodemap until it is filtered
for h in heads: for h in heads:
if not knownnode(h): if not knownnode(h):
allknown = False allknown = False
break break
if allknown: if allknown:
return (heads, False, heads) return (heads, False, heads)
res = setdiscovery.findcommonheads(repo.ui, repo, remote, res = setdiscovery.findcommonheads(repo.ui, repo, remote, heads,
abortwhenunrelated=not force, abortwhenunrelated=not force,
ancestorsof=ancestorsof) ancestorsof=ancestorsof)
common, anyinc, srvheads = res common, anyinc, srvheads = res
return (list(common), anyinc, heads or list(srvheads)) return (list(common), anyinc, heads or list(srvheads))
class outgoing(object): class outgoing(object):
'''Represents the set of nodes present in a local repo but not in a '''Represents the set of nodes present in a local repo but not in a
(possibly) remote one. (possibly) remote one.

mercurial/setdiscovery.py

return sample return sample
def _limitsample(sample, desiredlen): def _limitsample(sample, desiredlen):
"""return a random subset of sample of at most desiredlen item""" """return a random subset of sample of at most desiredlen item"""
if len(sample) > desiredlen: if len(sample) > desiredlen:
sample = set(random.sample(sample, desiredlen)) sample = set(random.sample(sample, desiredlen))
return sample return sample
def findcommonheads(ui, local, remote, def findcommonheads(ui, local, remote, heads=None,
initialsamplesize=100, initialsamplesize=100,
fullsamplesize=200, fullsamplesize=200,
abortwhenunrelated=True, abortwhenunrelated=True,
ancestorsof=None): ancestorsof=None):
'''Return a tuple (common, anyincoming, remoteheads) used to identify '''Return a tuple (common, anyincoming, remoteheads) used to identify
missing nodes from or in remote. missing nodes from or in remote.
''' '''
start = util.timer() start = util.timer()
roundtrips = 0 roundtrips = 0
cl = local.changelog cl = local.changelog
localsubset = None localsubset = None
if ancestorsof is not None: if ancestorsof is not None:
rev = local.changelog.rev rev = local.changelog.rev
localsubset = [rev(n) for n in ancestorsof] localsubset = [rev(n) for n in ancestorsof]
dag = dagutil.revlogdag(cl, localsubset=localsubset) dag = dagutil.revlogdag(cl, localsubset=localsubset)
# early exit if we know all the specified remote heads already # early exit if we know all the specified remote heads already
ui.debug("query 1; heads\n") ui.debug("query 1; heads\n")
roundtrips += 1 roundtrips += 1
ownheads = dag.heads() ownheads = dag.heads()
sample = _limitsample(ownheads, initialsamplesize) sample = _limitsample(ownheads, initialsamplesize)
# indices between sample and externalized version must match # indices between sample and externalized version must match
sample = list(sample) sample = list(sample)
if heads:
srvheadhashes = heads
yesno = remote.known(dag.externalizeall(sample))
else:
batch = remote.iterbatch() batch = remote.iterbatch()
batch.heads() batch.heads()
batch.known(dag.externalizeall(sample)) batch.known(dag.externalizeall(sample))
batch.submit() batch.submit()
srvheadhashes, yesno = batch.results() srvheadhashes, yesno = batch.results()
if cl.tip() == nullid: if cl.tip() == nullid:
if srvheadhashes != [nullid]: if srvheadhashes != [nullid]:
return [nullid], True, srvheadhashes return [nullid], True, srvheadhashes
return [nullid], False, [] return [nullid], False, []
# start actual discovery (we note this before the next "if" for # start actual discovery (we note this before the next "if" for
# compatibility reasons) # compatibility reasons)