Page MenuHomePhabricator

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

Authored by pulkit on Apr 24 2019, 2:38 PM.

Details

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 'Narrow:responsespec' 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.

  1. reponsespec clears that they are specs which are send as reponse by the

server

While I was here, I renamed narrowhgacl section to narrowacl as suggested by
idlsoft@ and martinvonz@.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

pulkit created this revision.Apr 24 2019, 2:38 PM

This is a new version of https://phab.mercurial-scm.org/D6218. I updated that one but it was not showing in Yadda as that's already marked as closed. So creating a new one.

martinvonz added inline comments.Apr 24 2019, 2:48 PM
hgext/narrow/narrowbundle2.py
161

Could you extract the name to a constant like _SPECPART etc? I'd also prefer it to start with narrow: like the other ones do (so maybe call it narrow:Spec?). Hmm, but narrow:spec and narrow:Spec are very likely to be confused, though. Perhaps this should be narrow:Clientspec or something? Or narrow:Serverspec? It's from the server, but it's for the client... Or can you think of a better name? Perhaps narrow:ResponseSpec or something like that?

idlsoft added inline comments.Apr 24 2019, 2:49 PM
hgext/narrow/narrowbundle2.py
168

shouldn't this be inside the if?
includepats and excludepats are not defined otherwise.

Ideally the new part would be documented in internals.bundle2. But other narrow parts aren't documented, so maybe we can hold off...

hgext/narrow/narrowbundle2.py
161

I don't believe the part name is capitalized in the decorator. Instead, the part name is capitalized when the part is added to the bundle. Please change (if necessary) to be consistent with the rest of the world.

171

Nit: use if x not in y.

pulkit edited the summary of this revision. (Show Details)Apr 25 2019, 1:30 PM
pulkit updated this revision to Diff 14919.

Ideally the new part would be documented in internals.bundle2. But other narrow parts aren't documented, so maybe we can hold off...

My guess is that they are not documented because narrow extension in EXPERIMENTAL.

martinvonz accepted this revision.EditedApr 25 2019, 1:38 PM

This looks good to me. Greg and Sandu?

This revision is now accepted and ready to land.Apr 25 2019, 1:38 PM

Also, this patch is meant for stable branch.

idlsoft added inline comments.Apr 25 2019, 2:10 PM
hgext/narrow/narrowbundle2.py
36

I thought this would have an uppercase letter to make it mandatory?

idlsoft added inline comments.Apr 25 2019, 2:12 PM
hgext/narrow/narrowbundle2.py
34

If we plan to change this to narrowacl at some point, perhaps now is the time, since it's a major version change.

pulkit added inline comments.Apr 26 2019, 6:00 PM
hgext/narrow/narrowbundle2.py
34

I am not sure if we want to do that. @martinvonz do we?

36

It's only required when we add the part to bundle2. See changes in exchange.py below.

martinvonz added inline comments.Apr 26 2019, 7:18 PM
hgext/narrow/narrowbundle2.py
34

Would make sense to me. "narrowhg" was the name of the extension. We changed most places to "narrow" when we moved it into core.

pulkit edited the summary of this revision. (Show Details)Apr 27 2019, 6:46 AM
pulkit updated this revision to Diff 14930.
This revision was automatically updated to reflect the committed changes.