[PATCH 2 of 2] wireproto: move wireproto capabilities computation in a subfunction

[Pierre-Yves David]

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 at ens-lyon.org wrote:
>>>> # HG changeset patch
>>>> # User Pierre-Yves David <pierre-yves.david at 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.

-- 
Pierre-Yves David