This is an archive of the discontinued Mercurial Phabricator instance.

Differential D4786

narrow: factor out logic to create cg while widening into separate fn
ClosedPublic

Authored by pulkit on Sep 28 2018, 4:58 PM.

Details

Reviewers
durin42
martinvonz
Group Reviewers
hg-reviewers
Commits
rHGad9ca365738b: narrow: factor out logic to create cg while widening into separate fn
Summary

This patch takes out the logic which generates a changegroup for widening a
narrow clone when ellipses are disabled. This is done because future patches
will introduce a narrow_widen() wireprotocol command which will send a
bundle2 with changegroup and will use this function.

The new function for now returns just the changegroup for compatibility with
existing code, but in future patches, when we establish a wireprotocol command
and call this function from there, this will return the required bundle2.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

pulkit created this revision.Sep 28 2018, 4:58 PM
martinvonz added a subscriber: martinvonz.Oct 1 2018, 2:27 AM
martinvonz added inline comments.
hgext/narrow/narrowbundle2.py
54

Do you plan to add support for ellipses to this too? I don't know if you need that feature, but I think the coming wire protocol command should eventually gain support for ellipsis nodes. Do you think a TODO about that makes sense? (I imagine it requires including the "known" set if the local repo has the ellipses requirement.)

pulkit added inline comments.Oct 1 2018, 10:57 AM
hgext/narrow/narrowbundle2.py
54

I do plan to support ellipses with the wireprotocol command. Since ellipses case will require sending a bundle not a changegroup, I plan to make that as a seperate function and call the required functions depending on ellipses is enabled or not.

Yeah the "known" set is needed. I also forgot to include that in wireprotocol command. Thanks!

pulkit edited the summary of this revision. (Show Details)Oct 1 2018, 12:01 PM
martinvonz added inline comments.Oct 1 2018, 12:08 PM
hgext/narrow/narrowbundle2.py
54

Since ellipses case will require sending a bundle not a changegroup

I'm not sure it will require that. The point of the "known" set was initially just to make sure that the server included all nodes that the client may have local commits based off of (see [1] and [2]). But then we realized that it would make more sense for widening and narrowing not to add or remove any ellipsis nodes. When interacting with our Google-internal server, that's how it actually works. I don't think I got around to making the core server completely trust the "known" set (or just the "common" set when not using ellipses), but I still think that's the right direction.

Maybe we should still wrap the returned changegroup in a bundle so we have more flexibility? It may be useful at least for letting the server include messages for the client. It's hard to say what else we may want to include in the bundle later.

[1] https://bitbucket.org/Google/narrowhg/commits/8c6dba960138b2758d6a37147d8338f751a7a05c
[2] https://bitbucket.org/Google/narrowhg/commits/ba65a969df547df0ccf26901bb3c5bd4e21445f2

pulkit edited the summary of this revision. (Show Details)Oct 2 2018, 10:19 AM
pulkit updated this revision to Diff 11559.
pulkit added a comment.Oct 2 2018, 10:30 AM

I have not changed this patch much, just added a XXX, saying we should return a bundle from here. I changed D4813 to return a bundle2 instead of just sending the changegroup and added a patch on top of it for returning a bundle2 from the new function introduced in this patch.

hgext/narrow/narrowbundle2.py
54

since ellipses case will require sending a bundle not a changegroup

I said that because there is a 'narrow:changespec' part being send from the server.

I still need to understand the ellipses case more in detail but I agree that "known" set can be helpful.

Maybe we should still wrap the returned changegroup in a bundle so we have more flexibility? It may be useful at least for letting the server include messages for the client. It's hard to say what else we may want to include in the bundle later.

This was a very good suggestion, thanks!

martinvonz accepted this revision.Oct 2 2018, 12:11 PM
martinvonz added inline comments.
hgext/narrow/narrowbundle2.py
54

I said that because there is a 'narrow:changespec' part being send from the server.

I assumed that was the reason :) But I don't think we'll need that part because we don't need to strip nodes in the ellipsis case either.

This revision is now accepted and ready to land.Oct 2 2018, 12:11 PM
Closed by commit rHGad9ca365738b: narrow: factor out logic to create cg while widening into separate fn (authored by pulkit). · Explain WhyOct 2 2018, 12:55 PM
This revision was automatically updated to reflect the committed changes.
pulkit added inline comments.Oct 3 2018, 9:41 AM
hgext/narrow/narrowbundle2.py
54

But I don't think we'll need that part because we don't need to strip nodes in the ellipsis case either.

I am still not able to understand the complete idea here, but preventing sending the kill signals and stripping will be very good. If I don't understand, I will try to understand this from you in upcoming sprint.

Revision Contents
Changeset List

Diff 11566

hgext/narrow/narrowbundle2.py

_MFHEADERSIZE = struct.calcsize(_ELIDEDMFHEADER) _MFHEADERSIZE = struct.calcsize(_ELIDEDMFHEADER)
# When advertising capabilities, always include narrow clone support. # When advertising capabilities, always include narrow clone support.
def getrepocaps_narrow(orig, repo, **kwargs): def getrepocaps_narrow(orig, repo, **kwargs):
caps = orig(repo, **kwargs) caps = orig(repo, **kwargs)
caps[NARROWCAP] = ['v0'] caps[NARROWCAP] = ['v0']
return caps return caps
def widen_bundle(repo, diffmatcher, common, known, cgversion, source, ellipses):
martinvonzUnsubmitted
Not Done

Do you plan to add support for ellipses to this too? I don't know if you need that feature, but I think the coming wire protocol command should eventually gain support for ellipsis nodes. Do you think a TODO about that makes sense? (I imagine it requires including the "known" set if the local repo has the ellipses requirement.)

martinvonz: Do you plan to add support for ellipses to this too? I don't know if you need that feature, but…
pulkitAuthorUnsubmitted
Not Done

I do plan to support ellipses with the wireprotocol command. Since ellipses case will require sending a bundle not a changegroup, I plan to make that as a seperate function and call the required functions depending on ellipses is enabled or not.

Yeah the "known" set is needed. I also forgot to include that in wireprotocol command. Thanks!

pulkit: I do plan to support ellipses with the wireprotocol command. Since ellipses case will require…
martinvonzUnsubmitted
Not Done

Since ellipses case will require sending a bundle not a changegroup

I'm not sure it will require that. The point of the "known" set was initially just to make sure that the server included all nodes that the client may have local commits based off of (see [1] and [2]). But then we realized that it would make more sense for widening and narrowing not to add or remove any ellipsis nodes. When interacting with our Google-internal server, that's how it actually works. I don't think I got around to making the core server completely trust the "known" set (or just the "common" set when not using ellipses), but I still think that's the right direction.

Maybe we should still wrap the returned changegroup in a bundle so we have more flexibility? It may be useful at least for letting the server include messages for the client. It's hard to say what else we may want to include in the bundle later.

[1] https://bitbucket.org/Google/narrowhg/commits/8c6dba960138b2758d6a37147d8338f751a7a05c
[2] https://bitbucket.org/Google/narrowhg/commits/ba65a969df547df0ccf26901bb3c5bd4e21445f2

martinvonz: > Since ellipses case will require sending a bundle not a changegroup I'm not sure it will…
pulkitAuthorUnsubmitted
Not Done

since ellipses case will require sending a bundle not a changegroup

I said that because there is a 'narrow:changespec' part being send from the server.

I still need to understand the ellipses case more in detail but I agree that "known" set can be helpful.

Maybe we should still wrap the returned changegroup in a bundle so we have more flexibility? It may be useful at least for letting the server include messages for the client. It's hard to say what else we may want to include in the bundle later.

This was a very good suggestion, thanks!

pulkit: > since ellipses case will require sending a bundle not a changegroup I said that because…
martinvonzUnsubmitted
Not Done

I said that because there is a 'narrow:changespec' part being send from the server.

I assumed that was the reason :) But I don't think we'll need that part because we don't need to strip nodes in the ellipsis case either.

martinvonz: > I said that because there is a 'narrow:changespec' part being send from the server. I…
pulkitAuthorUnsubmitted
Not Done

But I don't think we'll need that part because we don't need to strip nodes in the ellipsis case either.

I am still not able to understand the complete idea here, but preventing sending the kill signals and stripping will be very good. If I don't understand, I will try to understand this from you in upcoming sprint.

pulkit: > But I don't think we'll need that part because we don't need to strip nodes in the ellipsis…
"""generates changegroup for widening a narrow clone
repo is the localrepository instance
diffmatcher is a differencemacther of '(newincludes, newexcludes) -
(oldincludes, oldexcludes)'
common is set of common revs between server and client
known is a set of revs known on the client side (used in ellipses)
cgversion is the changegroup version to send
source is the command which called this codepath
ellipses is boolean value telling whether to send ellipses data or not
returns changegroup data of the changegroup built or return None if there
are no common revs
"""
# XXX: This patch will start sending bundle2 after couple of patches when
# called from the wireprotocol command
common = repo.revs("::%ln", common)
commonnodes = set()
cl = repo.changelog
for c in common:
commonnodes.add(cl.node(c))
if commonnodes:
# XXX: we should only send the filelogs (and treemanifest). user
# already has the changelog and manifest
packer = changegroup.getbundler(cgversion, repo,
filematcher=diffmatcher,
fullnodes=commonnodes)
cgdata = packer.generate(set([nullid]), list(commonnodes), False,
source, changelog=False)
return cgdata
return None
def getbundlechangegrouppart_widen(bundler, repo, source, bundlecaps=None, def getbundlechangegrouppart_widen(bundler, repo, source, bundlecaps=None,
b2caps=None, heads=None, common=None, b2caps=None, heads=None, common=None,
**kwargs): **kwargs):
"""Handling changegroup changegroup generation on the server when user """Handling changegroup changegroup generation on the server when user
is widening their narrowspec""" is widening their narrowspec"""
cgversions = b2caps.get('changegroup') cgversions = b2caps.get('changegroup')
if cgversions: # 3.1 and 3.2 ship with an empty value if cgversions: # 3.1 and 3.2 ship with an empty value
oldinclude = sorted(filter(bool, kwargs.get(r'oldincludepats', []))) oldinclude = sorted(filter(bool, kwargs.get(r'oldincludepats', [])))
oldexclude = sorted(filter(bool, kwargs.get(r'oldexcludepats', []))) oldexclude = sorted(filter(bool, kwargs.get(r'oldexcludepats', [])))
oldmatch = narrowspec.match(repo.root, include=oldinclude, oldmatch = narrowspec.match(repo.root, include=oldinclude,
exclude=oldexclude) exclude=oldexclude)
diffmatch = matchmod.differencematcher(newmatch, oldmatch) diffmatch = matchmod.differencematcher(newmatch, oldmatch)
common = set(common or [nullid]) common = set(common or [nullid])
if (oldinclude != include or oldexclude != exclude): if (oldinclude != include or oldexclude != exclude):
common = repo.revs("::%ln", common) cgdata = widen_bundle(repo, diffmatch, common, [], version,
commonnodes = set() source, False)
cl = repo.changelog if cgdata is not None:
for c in common:
commonnodes.add(cl.node(c))
if commonnodes:
# XXX: we should only send the filelogs (and treemanifest). user
# already has the changelog and manifest
packer = changegroup.getbundler(version, repo,
filematcher=diffmatch,
fullnodes=commonnodes)
cgdata = packer.generate(set([nullid]), list(commonnodes), False,
source, changelog=False)
part = bundler.newpart('changegroup', data=cgdata) part = bundler.newpart('changegroup', data=cgdata)
part.addparam('version', version) part.addparam('version', version)
if 'treemanifest' in repo.requirements: if 'treemanifest' in repo.requirements:
part.addparam('treemanifest', '1') part.addparam('treemanifest', '1')
# Serve a changegroup for a client with a narrow clone. # Serve a changegroup for a client with a narrow clone.
def getbundlechangegrouppart_narrow(bundler, repo, source, def getbundlechangegrouppart_narrow(bundler, repo, source,
bundlecaps=None, b2caps=None, heads=None, bundlecaps=None, b2caps=None, heads=None,