This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6218

narrow: send specs as bundle2 data instead of param (issue5952) (issue6019)
ClosedPublic

Authored by pulkit on Apr 8 2019, 11:23 AM.

Details

Reviewers
durin42
martinvonz
Group Reviewers
hg-reviewers
Commits
rHG8a37c9b6cf0a: narrow: send specs as bundle2 data instead of param (issue5952) (issue6019)
Summary

Before this patch, when ACL is involved, narrowspecs are send as bundle2
parameter for narrow:spec bundle2 part. The limitation of bundle2 parts are they
cannot send data larger than 255 bytes. Includes and excludes in narrow are not
limited by size and they can grow over 255 bytes.

This patch introduces a new mandatory bundle2 part and send narrowspecs as data
of that. The new bundle2 part is introduced to keep things cleaner and easy to
distinguish related to backward compatibility.
The part is mandatory because without server's narrowspec, the local ACL narrow
repo won't work.

This patch makes clients compatible with servers which have older versions.
However I left a comment that we should drop the other bundle2 part soon as
that's broken and people should not rely on that.

I named the new bundle2 part 'Narrowspec' because:

  1. Capital 'N' to make it mandatory
  2. 'Narrow:spec' cannot be used because bundle2 enforces that there should not

be two different parts which resolve to same name when lowercased.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

pulkit created this revision.Apr 8 2019, 11:23 AM
pulkit added inline comments.Apr 8 2019, 11:25 AM
mercurial/exchange.py
2216

I don't feel good about the fact that we are not encoding data here. Is there exists some function which I can use to encode and decode this list of specs?

martinvonz added inline comments.Apr 8 2019, 11:48 AM
mercurial/exchange.py
2216

You could probably reuse the function from D6184

pulkit updated this revision to Diff 14711.Apr 12 2019, 10:24 AM
martinvonz added inline comments.Apr 16 2019, 12:03 PM
hgext/narrow/narrowbundle2.py
156

I'd drop these checks

mercurial/exchange.py
2215–2216

And these

Closed by commit rHG8a37c9b6cf0a: narrow: send specs as bundle2 data instead of param (issue5952) (issue6019) (authored by pulkit). · Explain WhyApr 16 2019, 12:11 PM
This revision was automatically updated to reflect the committed changes.
pulkit added inline comments.Apr 16 2019, 1:11 PM
hgext/narrow/narrowbundle2.py
156

Sent D6241.

indygreg added a subscriber: indygreg.Apr 17 2019, 6:04 AM

This patch is backwards incompatible over the wire protocol.

The problem is a new client will blindly send part data to an old server expecting part parameters. The old server won't read the part data and it would be as if the includes and excludes were not sent.

We need some kind of capability negotiation that allows the client to opt in to the newer behavior if the server advertises support for it.

Also, my personal preference is to create new bundle2 parts rather than change behavior of existing bundle2 parts. Doing things this way ensures that behavior for a named bundle2 part is constant over time. This keeps implementations simpler, as individual part handling can do one thing and one thing only.

Finally, the internals help docs should be updated to reflect changes to bundle2 part behavior.

martinvonz added a comment.Apr 17 2019, 11:08 AM
In D6218#91058, @indygreg wrote:

This patch is backwards incompatible over the wire protocol.
The problem is a new client will blindly send part data to an old server expecting part parameters. The old server won't read the part data and it would be as if the includes and excludes were not sent.

It's an experimental feature and I suspect it's used only by Sandu (@idlsoft). Sandu, if we released this without the capability negotiation that Greg is talking about, you would need to make sure to upgrade the server before you upgrade your client(s). Are you okay with that? Is anyone aware of any other users of this feature? Greg, are you okay with making a breaking change (to an experimental feature) if the few existing users are okay with it?

We need some kind of capability negotiation that allows the client to opt in to the newer behavior if the server advertises support for it.
Also, my personal preference is to create new bundle2 parts rather than change behavior of existing bundle2 parts. Doing things this way ensures that behavior for a named bundle2 part is constant over time. This keeps implementations simpler, as individual part handling can do one thing and one thing only.
Finally, the internals help docs should be updated to reflect changes to bundle2 part behavior.

pulkit added a comment.Apr 17 2019, 11:22 AM
In D6218#91119, @martinvonz wrote:
In D6218#91058, @indygreg wrote:

This patch is backwards incompatible over the wire protocol.
The problem is a new client will blindly send part data to an old server expecting part parameters. The old server won't read the part data and it would be as if the includes and excludes were not sent.

It's an experimental feature and I suspect it's used only by Sandu (@idlsoft). Sandu, if we released this without the capability negotiation that Greg is talking about, you would need to make sure to upgrade the server before you upgrade your client(s). Are you okay with that? Is anyone aware of any other users of this feature? Greg, are you okay with making a breaking change (to an experimental feature) if the few existing users are okay with it?

I agree with @martinvonz here. narrow extension is experimental right now, in 4.9 we had a lot of breaking changes. The narrowspecs are only send back in case when ACL is enabled. If there are users who rely on existing behavior, they must have hit the bug just like @idlsoft hit.

I am not sure how we can keep sending narrowspecs back using bundle2 param and fix the issues which this patch is trying to.

We need some kind of capability negotiation that allows the client to opt in to the newer behavior if the server advertises support for it.
Also, my personal preference is to create new bundle2 parts rather than change behavior of existing bundle2 parts. Doing things this way ensures that behavior for a named bundle2 part is constant over time. This keeps implementations simpler, as individual part handling can do one thing and one thing only.
Finally, the internals help docs should be updated to reflect changes to bundle2 part behavior.

idlsoft added a comment.Apr 17 2019, 12:37 PM

Because the current client ignores the data completely, the only way to force it to fail I think is to change the name of the part.
This would make things cleaner probably, but I'll deal with whatever solution you guys settle on.

martinvonz added a comment.Apr 17 2019, 12:44 PM
In D6218#91121, @idlsoft wrote:

Because the current client ignores the data completely, the only way to force it to fail I think is to change the name of the part.
This would make things cleaner probably, but I'll deal with whatever solution you guys settle on.

I think bundle2 parts can be marked mandatory (by using uppercase in their name?). It seems to me like the ACL part should be mandatory. Is that correct, Sandu? So that's a good point and thanks for mentioning that. Pulkit, I think it's enough to change the name to be something like narrow:Spec or Narrow:spec (I'm thinking the former since some narrow parts are mandatory and some are not and then they all still start with narrow:).

idlsoft added a comment.Apr 17 2019, 1:09 PM

If ACL is enabled, processing this part is mandatory, yes.
On clone, or pull the user doesn't specify includes, so reading this part is the only way the client can get them.

idlsoft added a comment.Apr 18 2019, 10:43 AM

This is nitpicking, but there is a duplicate _NARROWACL_SECTION definition in narrowbundle2.py,
I think only the one in exchange.py should remain.
Btw it's still 'narrowhgacl' from the old days.

pulkit edited the summary of this revision. (Show Details)Apr 19 2019, 5:06 PM
pulkit updated this revision to Diff 14870.
pulkit added a comment.Apr 24 2019, 2:40 PM

I created a new version at D6310.

Revision Contents
Changeset List

PathPackages
Mhgext/narrow/narrowbundle2.py (20 lines)
Mmercurial/exchange.py (11 lines)

Diff 14870

hgext/narrow/narrowbundle2.py

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')
@bundle2.parthandler(_SPECPART, (_SPECPART_INCLUDE, _SPECPART_EXCLUDE)) @bundle2.parthandler(_SPECPART, (_SPECPART_INCLUDE, _SPECPART_EXCLUDE))
def _handlechangespec_2(op, inpart): def _handlechangespec_2(op, inpart):
# XXX: This bundle2 handling is buggy and should be removed after hg5.2 is
# released. New servers will send a mandatory bundle2 part named
# 'Narrowspec' and will send specs as data instead of params.
# Refer to issue5952 and 6019
includepats = set(inpart.params.get(_SPECPART_INCLUDE, '').splitlines()) includepats = set(inpart.params.get(_SPECPART_INCLUDE, '').splitlines())
excludepats = set(inpart.params.get(_SPECPART_EXCLUDE, '').splitlines()) excludepats = set(inpart.params.get(_SPECPART_EXCLUDE, '').splitlines())
narrowspec.validatepatterns(includepats) narrowspec.validatepatterns(includepats)
narrowspec.validatepatterns(excludepats) narrowspec.validatepatterns(excludepats)
if not repository.NARROW_REQUIREMENT in op.repo.requirements: if not repository.NARROW_REQUIREMENT in op.repo.requirements:
op.repo.requirements.add(repository.NARROW_REQUIREMENT) op.repo.requirements.add(repository.NARROW_REQUIREMENT)
op.repo._writerequirements() op.repo._writerequirements()
martinvonzUnsubmitted
Not Done

I'd drop these checks

martinvonz: I'd drop these checks
pulkitAuthorUnsubmitted
Not Done

Sent D6241.

pulkit: Sent D6241.
op.repo.setnarrowpats(includepats, excludepats) op.repo.setnarrowpats(includepats, excludepats)
narrowspec.copytoworkingcopy(op.repo) narrowspec.copytoworkingcopy(op.repo)
@bundle2.parthandler('Narrowspec')
def _handlenarrowspecs(op, inpart):
data = inpart.read()
if data:
inc, exc = data.split('\0')
includepats = set(inc.splitlines())
excludepats = set(exc.splitlines())
narrowspec.validatepatterns(includepats)
narrowspec.validatepatterns(excludepats)
if not repository.NARROW_REQUIREMENT in op.repo.requirements:
op.repo.requirements.add(repository.NARROW_REQUIREMENT)
op.repo._writerequirements()
op.repo.setnarrowpats(includepats, excludepats)
narrowspec.copytoworkingcopy(op.repo)
@bundle2.parthandler(_CHANGESPECPART) @bundle2.parthandler(_CHANGESPECPART)
def _handlechangespec(op, inpart): def _handlechangespec(op, inpart):
repo = op.repo repo = op.repo
cl = repo.changelog cl = repo.changelog
# changesets which need to be stripped entirely. either they're no longer # changesets which need to be stripped entirely. either they're no longer
# needed in the new narrow spec, or the server is sending a replacement # needed in the new narrow spec, or the server is sending a replacement
# in the changegroup part. # in the changegroup part.

mercurial/exchange.py

part.addparam('nbchanges', '%d' % len(outgoing.missing), part.addparam('nbchanges', '%d' % len(outgoing.missing),
mandatory=False) mandatory=False)
if 'treemanifest' in repo.requirements: if 'treemanifest' in repo.requirements:
part.addparam('treemanifest', '1') part.addparam('treemanifest', '1')
if (kwargs.get(r'narrow', False) and kwargs.get(r'narrow_acl', False) if (kwargs.get(r'narrow', False) and kwargs.get(r'narrow_acl', False)
and (include or exclude)): and (include or exclude)):
narrowspecpart = bundler.newpart('narrow:spec') # this is mandatory because otherwise ACL clients won't work
pulkitAuthorUnsubmitted
Not Done

I don't feel good about the fact that we are not encoding data here. Is there exists some function which I can use to encode and decode this list of specs?

pulkit: I don't feel good about the fact that we are not encoding data here. Is there exists some…
martinvonzUnsubmitted
Not Done

You could probably reuse the function from D6184

martinvonz: You could probably reuse the function from D6184
martinvonzUnsubmitted
Not Done

And these

martinvonz: And these
if include: narrowspecpart = bundler.newpart('Narrowspec')
narrowspecpart.addparam( narrowspecpart.data = '%s\0%s' % ('\n'.join(include),
'include', '\n'.join(include), mandatory=True) '\n'.join(exclude))
if exclude:
narrowspecpart.addparam(
'exclude', '\n'.join(exclude), mandatory=True)
@getbundle2partsgenerator('bookmarks') @getbundle2partsgenerator('bookmarks')
def _getbundlebookmarkpart(bundler, repo, source, bundlecaps=None, def _getbundlebookmarkpart(bundler, repo, source, bundlecaps=None,
b2caps=None, **kwargs): b2caps=None, **kwargs):
"""add a bookmark part to the requested bundle""" """add a bookmark part to the requested bundle"""
if not kwargs.get(r'bookmarks', False): if not kwargs.get(r'bookmarks', False):
return return
if 'bookmarks' not in b2caps: if 'bookmarks' not in b2caps: