D6218: narrow: send specs as bundle2 data instead of param (issue5952) (issue6019)
pulkit added a comment. I created a new version at https://phab.mercurial-scm.org/D6310. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D6218 To: pulkit, durin42, martinvonz, #hg-reviewers Cc: indygreg, idlsoft, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D6218: narrow: send specs as bundle2 data instead of param (issue5952) (issue6019)
pulkit updated this revision to Diff 14870.
pulkit edited the summary of this revision.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D6218?vs=14765=14870
REVISION DETAIL
https://phab.mercurial-scm.org/D6218
AFFECTED FILES
hgext/narrow/narrowbundle2.py
mercurial/exchange.py
CHANGE DETAILS
diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -2213,13 +2213,10 @@
if (kwargs.get(r'narrow', False) and kwargs.get(r'narrow_acl', False)
and (include or exclude)):
-narrowspecpart = bundler.newpart('narrow:spec')
-if include:
-narrowspecpart.addparam(
-'include', '\n'.join(include), mandatory=True)
-if exclude:
-narrowspecpart.addparam(
-'exclude', '\n'.join(exclude), mandatory=True)
+# this is mandatory because otherwise ACL clients won't work
+narrowspecpart = bundler.newpart('Narrowspec')
+narrowspecpart.data = '%s\0%s' % ('\n'.join(include),
+ '\n'.join(exclude))
@getbundle2partsgenerator('bookmarks')
def _getbundlebookmarkpart(bundler, repo, source, bundlecaps=None,
diff --git a/hgext/narrow/narrowbundle2.py b/hgext/narrow/narrowbundle2.py
--- a/hgext/narrow/narrowbundle2.py
+++ b/hgext/narrow/narrowbundle2.py
@@ -142,6 +142,10 @@
@bundle2.parthandler(_SPECPART, (_SPECPART_INCLUDE, _SPECPART_EXCLUDE))
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())
excludepats = set(inpart.params.get(_SPECPART_EXCLUDE, '').splitlines())
narrowspec.validatepatterns(includepats)
@@ -153,6 +157,22 @@
op.repo.setnarrowpats(includepats, excludepats)
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)
def _handlechangespec(op, inpart):
repo = op.repo
To: pulkit, durin42, martinvonz, #hg-reviewers
Cc: indygreg, idlsoft, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D6218: narrow: send specs as bundle2 data instead of param (issue5952) (issue6019)
idlsoft added a comment. 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. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D6218 To: pulkit, durin42, martinvonz, #hg-reviewers Cc: indygreg, idlsoft, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D6218: narrow: send specs as bundle2 data instead of param (issue5952) (issue6019)
idlsoft added a comment. 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. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D6218 To: pulkit, durin42, martinvonz, #hg-reviewers Cc: indygreg, idlsoft, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D6218: narrow: send specs as bundle2 data instead of param (issue5952) (issue6019)
martinvonz added a comment. In https://phab.mercurial-scm.org/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:`). REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D6218 To: pulkit, durin42, martinvonz, #hg-reviewers Cc: indygreg, idlsoft, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D6218: narrow: send specs as bundle2 data instead of param (issue5952) (issue6019)
idlsoft added a comment. 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. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D6218 To: pulkit, durin42, martinvonz, #hg-reviewers Cc: indygreg, idlsoft, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D6218: narrow: send specs as bundle2 data instead of param (issue5952) (issue6019)
pulkit added a comment. In https://phab.mercurial-scm.org/D6218#91119, @martinvonz wrote: > In https://phab.mercurial-scm.org/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. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D6218 To: pulkit, durin42, martinvonz, #hg-reviewers Cc: indygreg, idlsoft, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D6218: narrow: send specs as bundle2 data instead of param (issue5952) (issue6019)
martinvonz added a comment. In https://phab.mercurial-scm.org/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. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D6218 To: pulkit, durin42, martinvonz, #hg-reviewers Cc: indygreg, idlsoft, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D6218: narrow: send specs as bundle2 data instead of param (issue5952) (issue6019)
indygreg added a comment. 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. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D6218 To: pulkit, durin42, martinvonz, #hg-reviewers Cc: indygreg, idlsoft, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D6218: narrow: send specs as bundle2 data instead of param (issue5952) (issue6019)
pulkit added inline comments. INLINE COMMENTS > martinvonz wrote in narrowbundle2.py:152 > I'd drop these checks Sent https://phab.mercurial-scm.org/D6241. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D6218 To: pulkit, durin42, martinvonz, #hg-reviewers Cc: idlsoft, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D6218: narrow: send specs as bundle2 data instead of param (issue5952) (issue6019)
This revision was automatically updated to reflect the committed changes.
Closed by commit rHG8a37c9b6cf0a: narrow: send specs as bundle2 data instead of
param (issue5952) (issue6019) (authored by pulkit, committed by ).
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D6218?vs=14711=14765
REVISION DETAIL
https://phab.mercurial-scm.org/D6218
AFFECTED FILES
hgext/narrow/narrowbundle2.py
mercurial/exchange.py
CHANGE DETAILS
diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -2214,12 +2214,14 @@
if (kwargs.get(r'narrow', False) and kwargs.get(r'narrow_acl', False)
and (include or exclude)):
narrowspecpart = bundler.newpart('narrow:spec')
+data = ''
if include:
-narrowspecpart.addparam(
-'include', '\n'.join(include), mandatory=True)
+data += '\n'.join(include)
+data += '\0'
if exclude:
-narrowspecpart.addparam(
-'exclude', '\n'.join(exclude), mandatory=True)
+data += '\n'.join(exclude)
+
+narrowspecpart.data = data
@getbundle2partsgenerator('bookmarks')
def _getbundlebookmarkpart(bundler, repo, source, bundlecaps=None,
diff --git a/hgext/narrow/narrowbundle2.py b/hgext/narrow/narrowbundle2.py
--- a/hgext/narrow/narrowbundle2.py
+++ b/hgext/narrow/narrowbundle2.py
@@ -144,6 +144,15 @@
def _handlechangespec_2(op, inpart):
includepats = set(inpart.params.get(_SPECPART_INCLUDE, '').splitlines())
excludepats = set(inpart.params.get(_SPECPART_EXCLUDE, '').splitlines())
+data = inpart.read()
+# old servers don't send includes and excludes using bundle2 data, they use
+# bundle2 parameters instead.
+if data:
+inc, exc = data.split('\0')
+if inc:
+includepats |= set(inc.splitlines())
+if exc:
+excludepats |= set(exc.splitlines())
narrowspec.validatepatterns(includepats)
narrowspec.validatepatterns(excludepats)
To: pulkit, durin42, martinvonz, #hg-reviewers
Cc: idlsoft, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D6218: narrow: send specs as bundle2 data instead of param (issue5952) (issue6019)
martinvonz added inline comments.
INLINE COMMENTS
> narrowbundle2.py:152
> +inc, exc = data.split('\0')
> +if inc:
> +includepats |= set(inc.splitlines())
I'd drop these checks
> exchange.py:2218
> +data = ''
> if include:
> +data += '\n'.join(include)
And these
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D6218
To: pulkit, durin42, martinvonz, #hg-reviewers
Cc: idlsoft, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D6218: narrow: send specs as bundle2 data instead of param (issue5952) (issue6019)
pulkit updated this revision to Diff 14711.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D6218?vs=14691=14711
REVISION DETAIL
https://phab.mercurial-scm.org/D6218
AFFECTED FILES
hgext/narrow/narrowbundle2.py
mercurial/exchange.py
CHANGE DETAILS
diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -2214,12 +2214,14 @@
if (kwargs.get(r'narrow', False) and kwargs.get(r'narrow_acl', False)
and (include or exclude)):
narrowspecpart = bundler.newpart('narrow:spec')
+data = ''
if include:
-narrowspecpart.addparam(
-'include', '\n'.join(include), mandatory=True)
+data += '\n'.join(include)
+data += '\0'
if exclude:
-narrowspecpart.addparam(
-'exclude', '\n'.join(exclude), mandatory=True)
+data += '\n'.join(exclude)
+
+narrowspecpart.data = data
@getbundle2partsgenerator('bookmarks')
def _getbundlebookmarkpart(bundler, repo, source, bundlecaps=None,
diff --git a/hgext/narrow/narrowbundle2.py b/hgext/narrow/narrowbundle2.py
--- a/hgext/narrow/narrowbundle2.py
+++ b/hgext/narrow/narrowbundle2.py
@@ -144,6 +144,15 @@
def _handlechangespec_2(op, inpart):
includepats = set(inpart.params.get(_SPECPART_INCLUDE, '').splitlines())
excludepats = set(inpart.params.get(_SPECPART_EXCLUDE, '').splitlines())
+data = inpart.read()
+# old servers don't send includes and excludes using bundle2 data, they use
+# bundle2 parameters instead.
+if data:
+inc, exc = data.split('\0')
+if inc:
+includepats |= set(inc.splitlines())
+if exc:
+excludepats |= set(exc.splitlines())
narrowspec.validatepatterns(includepats)
narrowspec.validatepatterns(excludepats)
To: pulkit, durin42, martinvonz, #hg-reviewers
Cc: idlsoft, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D6218: narrow: send specs as bundle2 data instead of param (issue5952) (issue6019)
martinvonz added inline comments. INLINE COMMENTS > pulkit wrote in exchange.py:2217 > 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? You could probably reuse the function from https://phab.mercurial-scm.org/D6184 REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D6218 To: pulkit, durin42, martinvonz, #hg-reviewers Cc: mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D6218: narrow: send specs as bundle2 data instead of param (issue5952) (issue6019)
pulkit added inline comments.
INLINE COMMENTS
> exchange.py:2217
> narrowspecpart = bundler.newpart('narrow:spec')
> +data = ''
> if include:
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?
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D6218
To: pulkit, durin42, martinvonz, #hg-reviewers
Cc: mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D6218: narrow: send specs as bundle2 data instead of param (issue5952) (issue6019)
pulkit created this revision.
Herald added a reviewer: durin42.
Herald added a reviewer: martinvonz.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.
REVISION 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 start sending them as bundle2 data. After this change, we try to
read
specs both from parameters and data, making it compatible with older servers.
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D6218
AFFECTED FILES
hgext/narrow/narrowbundle2.py
mercurial/exchange.py
CHANGE DETAILS
diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -2214,12 +2214,14 @@
if (kwargs.get(r'narrow', False) and kwargs.get(r'narrow_acl', False)
and (include or exclude)):
narrowspecpart = bundler.newpart('narrow:spec')
+data = ''
if include:
-narrowspecpart.addparam(
-'include', '\n'.join(include), mandatory=True)
+data += '\n'.join(include)
+data += '\0'
if exclude:
-narrowspecpart.addparam(
-'exclude', '\n'.join(exclude), mandatory=True)
+data += '\n'.join(exclude)
+
+narrowspecpart.data = data
@getbundle2partsgenerator('bookmarks')
def _getbundlebookmarkpart(bundler, repo, source, bundlecaps=None,
diff --git a/hgext/narrow/narrowbundle2.py b/hgext/narrow/narrowbundle2.py
--- a/hgext/narrow/narrowbundle2.py
+++ b/hgext/narrow/narrowbundle2.py
@@ -144,6 +144,12 @@
def _handlechangespec_2(op, inpart):
includepats = set(inpart.params.get(_SPECPART_INCLUDE, '').splitlines())
excludepats = set(inpart.params.get(_SPECPART_EXCLUDE, '').splitlines())
+data = inpart.read()
+inc, exc = data.split('\0')
+if inc:
+includepats |= set(inc.splitlines())
+if exc:
+excludepats |= set(exc.splitlines())
narrowspec.validatepatterns(includepats)
narrowspec.validatepatterns(excludepats)
To: pulkit, durin42, martinvonz, #hg-reviewers
Cc: mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
