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

login
register
mail settings
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

Pierre-Yves David - March 13, 2014, 6:48 p.m.
# 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.
Matt Mackall - March 13, 2014, 9:43 p.m.
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.
David Soria Parra - March 13, 2014, 10:27 p.m.
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.
Pierre-Yves David - March 14, 2014, 4:41 a.m.
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.
Matt Mackall - March 16, 2014, 10:40 p.m.
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.)
Pierre-Yves David - March 17, 2014, 5:45 p.m.
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')