Patchwork [3,of,5] bundle2.bundlepart: allow setting mandatory explicitly

login
register
mail settings
Submitter Eric Sumner
Date Dec. 16, 2014, 9:57 p.m.
Message ID <cc5b8da4adb3c56a2485.1418767052@dev911.prn1.facebook.com>
Download mbox | patch
Permalink /patch/7125/
State Changes Requested
Delegated to: Pierre-Yves David
Headers show

Comments

Eric Sumner - Dec. 16, 2014, 9:57 p.m.
# HG changeset patch
# User Eric Sumner <ericsumner@fb.com>
# Date 1418424701 28800
#      Fri Dec 12 14:51:41 2014 -0800
# Node ID cc5b8da4adb3c56a2485e177498e036badfb21c7
# Parent  db0ab9e45c6d967d0e0c07de24db77d84266de33
bundle2.bundlepart: allow setting mandatory explicitly

Encoding whether or not a part is mandatory in the capitalization of the
parttype is unintuitive and error-prone.  This sequence of patches separates
these concerns in the API to reduce programmer error and pave the way for
a potential change in how this information is transmitted over the wire.

This patch adds a 'mandatory' property to bundle2.bundlepart that can be used
to set a part as mandatory or optional indepenently of the parttype name.  If
it is not set explicitly, the code falls back to detecting it from the parttype
case.
Pierre-Yves David - Dec. 17, 2014, 1:08 a.m.
On 12/16/2014 01:57 PM, Eric Sumner wrote:
> # HG changeset patch
> # User Eric Sumner <ericsumner@fb.com>
> # Date 1418424701 28800
> #      Fri Dec 12 14:51:41 2014 -0800
> # Node ID cc5b8da4adb3c56a2485e177498e036badfb21c7
> # Parent  db0ab9e45c6d967d0e0c07de24db77d84266de33
> bundle2.bundlepart: allow setting mandatory explicitly
>
> Encoding whether or not a part is mandatory in the capitalization of the
> parttype is unintuitive and error-prone.  This sequence of patches separates
> these concerns in the API to reduce programmer error and pave the way for
> a potential change in how this information is transmitted over the wire.
>
> This patch adds a 'mandatory' property to bundle2.bundlepart that can be used
> to set a part as mandatory or optional indepenently of the parttype name.  If
> it is not set explicitly, the code falls back to detecting it from the parttype
> case.
>
> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
> --- a/mercurial/bundle2.py
> +++ b/mercurial/bundle2.py
> @@ -588,7 +588,7 @@
>       """
>
>       def __init__(self, parttype, mandatoryparams=(), advisoryparams=(),
> -                 data=''):
> +                 data='', mandatory = 'UNSPECIFIED'):
>           self.id = None
>           self.type = parttype
>           self._data = data
> @@ -605,6 +605,10 @@
>           # - False: currently generated,
>           # - True: generation done.
>           self._generated = None
> +        if mandatory is 'UNSPECIFIED':
> +            self._mandatory = None
> +        else:
> +            self._mandatory = bool(mandatory)

I would probably take advantage of the experimental status of bundle2 
and just make mandatory the default.

>       # methods used to defines the part content
>       def __setdata(self, data):
> @@ -625,6 +629,18 @@
>           # make it an immutable tuple to force people through ``addparam``
>           return tuple(self._advisoryparams)
>
> +    @property
> +    def mandatory(self):
> +        if self._mandatory is None:
> +            # remove this autodetect once everything uses the new API
> +            return self.type != self.type.lower()
> +        else:
> +            return self._mandatory
> +
> +    @mandatory.setter
> +    def mandatory(self, value):
> +        self._mandatory = bool(value)

This property business seems unecesary and should be dropped.
Also, I thin the .setter thing is not in python 2.4…
Mike Hommey - Dec. 17, 2014, 1:15 a.m.
On Tue, Dec 16, 2014 at 05:08:12PM -0800, Pierre-Yves David wrote:
> >+    @mandatory.setter
> >+    def mandatory(self, value):
> >+        self._mandatory = bool(value)
> 
> This property business seems unecesary and should be dropped.
> Also, I thin the .setter thing is not in python 2.4…

It's new to 2.6, so even after 2.4 support is dropped, it can't be used.

Mike

Patch

diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
--- a/mercurial/bundle2.py
+++ b/mercurial/bundle2.py
@@ -588,7 +588,7 @@ 
     """
 
     def __init__(self, parttype, mandatoryparams=(), advisoryparams=(),
-                 data=''):
+                 data='', mandatory = 'UNSPECIFIED'):
         self.id = None
         self.type = parttype
         self._data = data
@@ -605,6 +605,10 @@ 
         # - False: currently generated,
         # - True: generation done.
         self._generated = None
+        if mandatory is 'UNSPECIFIED':
+            self._mandatory = None
+        else:
+            self._mandatory = bool(mandatory)
 
     # methods used to defines the part content
     def __setdata(self, data):
@@ -625,6 +629,18 @@ 
         # make it an immutable tuple to force people through ``addparam``
         return tuple(self._advisoryparams)
 
+    @property
+    def mandatory(self):
+        if self._mandatory is None:
+            # remove this autodetect once everything uses the new API
+            return self.type != self.type.lower()
+        else:
+            return self._mandatory
+
+    @mandatory.setter
+    def mandatory(self, value):
+        self._mandatory = bool(value)
+
     def addparam(self, name, value='', mandatory=True):
         if self._generated is not None:
             raise error.ReadOnlyPartError('part is being generated')
@@ -642,9 +658,13 @@ 
             raise RuntimeError('part can only be consumed once')
         self._generated = False
         #### header
+        if self.mandatory:
+            parttype = self.type.upper()
+        else:
+            parttype = self.type.lower()
         ## parttype
-        header = [_pack(_fparttypesize, len(self.type)),
-                  self.type, _pack(_fpartid, self.id),
+        header = [_pack(_fparttypesize, len(parttype)),
+                  parttype, _pack(_fpartid, self.id),
                  ]
         ## parameters
         # count