| Submitter | Pierre-Yves David |
|---|---|
| Date | March 13, 2014, 6:48 p.m. |
| Message ID | <ccf1552655b90cac5157.1394736511@marginatus.alto.octopoid.net> |
| Download | mbox | patch |
| Permalink | /patch/3939/ |
| State | Superseded |
| Commit | 982f13bef5031a7a893b9404b027fdc7540f9d86 |
| Headers | show |
Comments
On Thu, 2014-03-13 at 11:48 -0700, pierre-yves.david@ens-lyon.org wrote: > # HG changeset patch > # User Pierre-Yves David <pierre-yves.david@fb.com> > # Date 1394660801 25200 > # Wed Mar 12 14:46:41 2014 -0700 > # Node ID ccf1552655b90cac5157e86fc914a0a33c46ea24 > # Parent 1bfa5d82da6540e6559c3396c692db848ea650f2 > wireproto: move wireproto capabilities computation in a subfunction > WIREPROTOCAPS = ['lookup', 'changegroupsubset', 'branchmap', 'pushkey', > 'known', 'getbundle', 'unbundlehash', 'batch'] Lowercase this, please. It's not C nor is it a constant.
pierre-yves.david@ens-lyon.org writes: > # HG changeset patch > # User Pierre-Yves David <pierre-yves.david@fb.com> > # Date 1394660801 25200 > # Wed Mar 12 14:46:41 2014 -0700 > # Node ID ccf1552655b90cac5157e86fc914a0a33c46ea24 > # Parent 1bfa5d82da6540e6559c3396c692db848ea650f2 > wireproto: move wireproto capabilities computation in a subfunction > > It will help people that need to add capabilities (in a more subtle was that > just adding some to the list) in multiple way: > > 1. This function returns a list, not a string. Making it easier to look at, > extend or alter the content. > > 2. The original capabilities function will be store in the dictionary of wire > protocol command. So extension that wrap this function also need to update > the dictionary entry. > > Both wrapping and update of the dictionary entry are needed because the > `hello` wire protocol use the function itself. This is specifically sneaky for > extension writer as ssh use the `hello` command while http use the > `capabilities` command. > > With this new `_capabilities` function there is one and only one obvious > place to wrap when needed. > > diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py > --- a/mercurial/wireproto.py > +++ b/mercurial/wireproto.py > @@ -415,11 +415,20 @@ def branches(repo, proto, nodes): > return "".join(r) > > > WIREPROTOCAPS = ['lookup', 'changegroupsubset', 'branchmap', 'pushkey', > 'known', 'getbundle', 'unbundlehash', 'batch'] > -def capabilities(repo, proto): > + > +def _capabilities(repo, proto): > + """return a list of capabilities for a repo > + > + This function exist to easy wrapping process from extension Maybe it's because I am not a native speaker but I honestly have a hard time understanding what you mean. > + - returns a lists: easy to alter > + - change done here will be progated to both `capabilities` and `hello` > + command without any other effort. without any other action needed. > + """ my dictionary doesn't find "progated". > # copy to prevent modification of the global list > caps = list(WIREPROTOCAPS) > if _allowstream(repo.ui): > if repo.ui.configbool('server', 'preferuncompressed', False): > caps.append('stream-preferred') > @@ -430,10 +439,16 @@ def capabilities(repo, proto): > # otherwise, add 'streamreqs' detailing our local revlog format > else: > caps.append('streamreqs=%s' % ','.join(requiredformats)) > caps.append('unbundle=%s' % ','.join(changegroupmod.bundlepriority)) > caps.append('httpheader=1024') > + return caps > + > +# If you are writting and extension and consider wrapping this function. Wrap > +# `_capabilities` instead. > +def capabilities(repo, proto): > + caps = _capabilities(repo, proto) why not just return ' '.join(_capabilities(repo,proto)) ? besodes the nits it looks good to me.
On 03/13/2014 02:43 PM, Matt Mackall wrote: > On Thu, 2014-03-13 at 11:48 -0700, pierre-yves.david@ens-lyon.org wrote: >> # HG changeset patch >> # User Pierre-Yves David <pierre-yves.david@fb.com> >> # Date 1394660801 25200 >> # Wed Mar 12 14:46:41 2014 -0700 >> # Node ID ccf1552655b90cac5157e86fc914a0a33c46ea24 >> # Parent 1bfa5d82da6540e6559c3396c692db848ea650f2 >> wireproto: move wireproto capabilities computation in a subfunction > >> WIREPROTOCAPS = ['lookup', 'changegroupsubset', 'branchmap', 'pushkey', >> 'known', 'getbundle', 'unbundlehash', 'batch'] > > Lowercase this, please. It's not C nor is it a constant. This mimic the naming in local repo. I assume ou want a patch to change the case in local repo too.
On Thu, 2014-03-13 at 21:41 -0700, Pierre-Yves David wrote: > > On 03/13/2014 02:43 PM, Matt Mackall wrote: > > On Thu, 2014-03-13 at 11:48 -0700, pierre-yves.david@ens-lyon.org wrote: > >> # HG changeset patch > >> # User Pierre-Yves David <pierre-yves.david@fb.com> > >> # Date 1394660801 25200 > >> # Wed Mar 12 14:46:41 2014 -0700 > >> # Node ID ccf1552655b90cac5157e86fc914a0a33c46ea24 > >> # Parent 1bfa5d82da6540e6559c3396c692db848ea650f2 > >> wireproto: move wireproto capabilities computation in a subfunction > > > >> WIREPROTOCAPS = ['lookup', 'changegroupsubset', 'branchmap', 'pushkey', > >> 'known', 'getbundle', 'unbundlehash', 'batch'] > > > > Lowercase this, please. It's not C nor is it a constant. > > This mimic the naming in local repo. Do you intend it to be constant, or no? That's the only reason you'd ever name something all caps, and that reason is not in the coding style. And I'm afraid it looks REALLY attractive to just extend this rather than wrapping a function, so I think it's not actually going to succeed in being constant. (FYI, largefiles is already broken here: it mangles proto caps globally rather than per-repo when enabled in just one hgwebdir repo. This can cause some interesting bugs depending on the order clients visit repos.)
On 03/16/2014 03:40 PM, Matt Mackall wrote: > On Thu, 2014-03-13 at 21:41 -0700, Pierre-Yves David wrote: >> >> On 03/13/2014 02:43 PM, Matt Mackall wrote: >>> On Thu, 2014-03-13 at 11:48 -0700, pierre-yves.david@ens-lyon.org wrote: >>>> # HG changeset patch >>>> # User Pierre-Yves David <pierre-yves.david@fb.com> >>>> # Date 1394660801 25200 >>>> # Wed Mar 12 14:46:41 2014 -0700 >>>> # Node ID ccf1552655b90cac5157e86fc914a0a33c46ea24 >>>> # Parent 1bfa5d82da6540e6559c3396c692db848ea650f2 >>>> wireproto: move wireproto capabilities computation in a subfunction >>> >>>> WIREPROTOCAPS = ['lookup', 'changegroupsubset', 'branchmap', 'pushkey', >>>> 'known', 'getbundle', 'unbundlehash', 'batch'] >>> >>> Lowercase this, please. It's not C nor is it a constant. >> >> This mimic the naming in local repo. > > Do you intend it to be constant, or no? That's the only reason you'd > ever name something all caps, and that reason is not in the coding > style. And I'm afraid it looks REALLY attractive to just extend this > rather than wrapping a function, so I think it's not actually going to > succeed in being constant. Currently, this is definitly not a constant otherwise it would be tuple and frozenset instead of list and set. > (FYI, largefiles is already broken here: it mangles proto caps globally > rather than per-repo when enabled in just one hgwebdir repo. This can > cause some interesting bugs depending on the order clients visit repos.) True, Extension escaping to other repo is a common issue. Its not limited to capabilities though. However. I sure that if we made it a strict constant (using a tuple) people will work around this by making the variable pointing to a new augmented tuple. This will lead to more subtle bug than the current one. I guess that right way to prevent people to misuse it proper documentation around it. Pointing to wrapping _capabilities instead. I still would like to keep the base list easy to access. This make it simple for extension that support multiple version of mercurial to see if functionality are supported into core or if they need to inject there own version of it. Moreover having both wrapping option available probably makes my live in the evolve extension easier.
Patch
diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py --- a/mercurial/wireproto.py +++ b/mercurial/wireproto.py @@ -415,11 +415,20 @@ def branches(repo, proto, nodes): return "".join(r) WIREPROTOCAPS = ['lookup', 'changegroupsubset', 'branchmap', 'pushkey', 'known', 'getbundle', 'unbundlehash', 'batch'] -def capabilities(repo, proto): + +def _capabilities(repo, proto): + """return a list of capabilities for a repo + + This function exist to easy wrapping process from extension + + - returns a lists: easy to alter + - change done here will be progated to both `capabilities` and `hello` + command without any other effort. without any other action needed. + """ # copy to prevent modification of the global list caps = list(WIREPROTOCAPS) if _allowstream(repo.ui): if repo.ui.configbool('server', 'preferuncompressed', False): caps.append('stream-preferred') @@ -430,10 +439,16 @@ def capabilities(repo, proto): # otherwise, add 'streamreqs' detailing our local revlog format else: caps.append('streamreqs=%s' % ','.join(requiredformats)) caps.append('unbundle=%s' % ','.join(changegroupmod.bundlepriority)) caps.append('httpheader=1024') + return caps + +# If you are writting and extension and consider wrapping this function. Wrap +# `_capabilities` instead. +def capabilities(repo, proto): + caps = _capabilities(repo, proto) return ' '.join(caps) def changegroup(repo, proto, roots): nodes = decodelist(roots) cg = repo.changegroup(nodes, 'serve')