Patchwork [1,of,4,V2] flagutil: introduce a flagprocessorsmixin class

login
register
mail settings
Submitter Pierre-Yves David
Date Aug. 11, 2019, 11:10 a.m.
Message ID <e6d22ec889461dbe819e.1565521847@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/41242/
State New
Headers show

Comments

Pierre-Yves David - Aug. 11, 2019, 11:10 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@octobus.net>
# Date 1565219568 -7200
#      Thu Aug 08 01:12:48 2019 +0200
# Node ID e6d22ec889461dbe819e0dbf4d2f0167736b2416
# Parent  6d61be152c5515fed315885f6c9cf9defe73de71
# EXP-Topic flag-processors
# Available At https://bitbucket.org/octobus/mercurial-devel/
#              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r e6d22ec88946
flagutil: introduce a flagprocessorsmixin class

To avoid code duplication, we will provide a simple "ready to use" mixin that
carry the appropriate logic. First we use it in standard revlog, we'll remove
code duplication in later changesets.
Yuya Nishihara - Aug. 31, 2019, 3:02 a.m.
On Sun, 11 Aug 2019 13:10:47 +0200, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@octobus.net>
> # Date 1565219568 -7200
> #      Thu Aug 08 01:12:48 2019 +0200
> # Node ID e6d22ec889461dbe819e0dbf4d2f0167736b2416
> # Parent  6d61be152c5515fed315885f6c9cf9defe73de71
> # EXP-Topic flag-processors
> # Available At https://bitbucket.org/octobus/mercurial-devel/
> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r e6d22ec88946
> flagutil: introduce a flagprocessorsmixin class

CC +indygreg

> diff --git a/mercurial/revlogutils/flagutil.py b/mercurial/revlogutils/flagutil.py
> --- a/mercurial/revlogutils/flagutil.py
> +++ b/mercurial/revlogutils/flagutil.py
> @@ -78,3 +78,74 @@ def insertflagprocessor(flag, processor,
>          msg = _("cannot register multiple processors on flag '%#x'.") % (flag)
>          raise error.Abort(msg)
>      flagprocessors[flag] = processor
> +
> +class flagprocessorsmixin(object):
> +    """basic mixin to support revlog flag processing
> +
> +    Make sure the `_flagprocessors` attribute is set at ``__init__`` time.

I feel a plain function taking (revlog, flagprocessors, ...) is simpler
than the mixin class which implicitly depends on revlog attributes. But
maybe I'm biased. I don't like class inheritance in general.
Pierre-Yves David - Sept. 2, 2019, 1:03 p.m.
On 8/31/19 5:02 AM, Yuya Nishihara wrote:
> On Sun, 11 Aug 2019 13:10:47 +0200, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@octobus.net>
>> # Date 1565219568 -7200
>> #      Thu Aug 08 01:12:48 2019 +0200
>> # Node ID e6d22ec889461dbe819e0dbf4d2f0167736b2416
>> # Parent  6d61be152c5515fed315885f6c9cf9defe73de71
>> # EXP-Topic flag-processors
>> # Available At https://bitbucket.org/octobus/mercurial-devel/
>> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r e6d22ec88946
>> flagutil: introduce a flagprocessorsmixin class
> 
> CC +indygreg
> 
>> diff --git a/mercurial/revlogutils/flagutil.py b/mercurial/revlogutils/flagutil.py
>> --- a/mercurial/revlogutils/flagutil.py
>> +++ b/mercurial/revlogutils/flagutil.py
>> @@ -78,3 +78,74 @@ def insertflagprocessor(flag, processor,
>>           msg = _("cannot register multiple processors on flag '%#x'.") % (flag)
>>           raise error.Abort(msg)
>>       flagprocessors[flag] = processor
>> +
>> +class flagprocessorsmixin(object):
>> +    """basic mixin to support revlog flag processing
>> +
>> +    Make sure the `_flagprocessors` attribute is set at ``__init__`` time.
> 
> I feel a plain function taking (revlog, flagprocessors, ...) is simpler
> than the mixin class which implicitly depends on revlog attributes. But
> maybe I'm biased. I don't like class inheritance in general.

Are you suggesting (revlog, currently_supported_flags, …)? If so, I am 
afraid this would lead to wrong API usage. It is important that the 
revlog bear the supported flags to help enforcing extension/config 
isolation. Maybe we could still use an external function but still rely 
on the attribute ?

In all case, can we move forward with this series and adjust in a follow 
up, I have a lot of other changes waiting on this ?
Yuya Nishihara - Sept. 4, 2019, 11:43 p.m.
On Mon, 2 Sep 2019 15:03:00 +0200, Pierre-Yves David wrote:
> On 8/31/19 5:02 AM, Yuya Nishihara wrote:
> > On Sun, 11 Aug 2019 13:10:47 +0200, Pierre-Yves David wrote:
> >> # HG changeset patch
> >> # User Pierre-Yves David <pierre-yves.david@octobus.net>
> >> # Date 1565219568 -7200
> >> #      Thu Aug 08 01:12:48 2019 +0200
> >> # Node ID e6d22ec889461dbe819e0dbf4d2f0167736b2416
> >> # Parent  6d61be152c5515fed315885f6c9cf9defe73de71
> >> # EXP-Topic flag-processors
> >> # Available At https://bitbucket.org/octobus/mercurial-devel/
> >> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r e6d22ec88946
> >> flagutil: introduce a flagprocessorsmixin class
> > 
> > CC +indygreg
> > 
> >> diff --git a/mercurial/revlogutils/flagutil.py b/mercurial/revlogutils/flagutil.py
> >> --- a/mercurial/revlogutils/flagutil.py
> >> +++ b/mercurial/revlogutils/flagutil.py
> >> @@ -78,3 +78,74 @@ def insertflagprocessor(flag, processor,
> >>           msg = _("cannot register multiple processors on flag '%#x'.") % (flag)
> >>           raise error.Abort(msg)
> >>       flagprocessors[flag] = processor
> >> +
> >> +class flagprocessorsmixin(object):
> >> +    """basic mixin to support revlog flag processing
> >> +
> >> +    Make sure the `_flagprocessors` attribute is set at ``__init__`` time.
> > 
> > I feel a plain function taking (revlog, flagprocessors, ...) is simpler
> > than the mixin class which implicitly depends on revlog attributes. But
> > maybe I'm biased. I don't like class inheritance in general.
> 
> Are you suggesting (revlog, currently_supported_flags, …)? If so, I am 
> afraid this would lead to wrong API usage.

Probably yes.

> It is important that the 
> revlog bear the supported flags to help enforcing extension/config 
> isolation.

Anyway, the user of the mixin would have to set self._flagprocessors
properly. I don't see much difference between mixin + attributes and
passing it as a function argument.

> In all case, can we move forward with this series and adjust in a follow 
> up, I have a lot of other changes waiting on this ?

If someone else agree with this change, please go ahead and queue this.
I only have a _minor_ concern, and I don't have expertise around this code.
Pierre-Yves David - Sept. 5, 2019, 8:24 a.m.
On 9/5/19 1:43 AM, Yuya Nishihara wrote:
> On Mon, 2 Sep 2019 15:03:00 +0200, Pierre-Yves David wrote:
>> On 8/31/19 5:02 AM, Yuya Nishihara wrote:
>>> On Sun, 11 Aug 2019 13:10:47 +0200, Pierre-Yves David wrote:
>>>> # HG changeset patch
>>>> # User Pierre-Yves David <pierre-yves.david@octobus.net>
>>>> # Date 1565219568 -7200
>>>> #      Thu Aug 08 01:12:48 2019 +0200
>>>> # Node ID e6d22ec889461dbe819e0dbf4d2f0167736b2416
>>>> # Parent  6d61be152c5515fed315885f6c9cf9defe73de71
>>>> # EXP-Topic flag-processors
>>>> # Available At https://bitbucket.org/octobus/mercurial-devel/
>>>> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r e6d22ec88946
>>>> flagutil: introduce a flagprocessorsmixin class
>>>
>>> CC +indygreg
>>>
>>>> diff --git a/mercurial/revlogutils/flagutil.py b/mercurial/revlogutils/flagutil.py
>>>> --- a/mercurial/revlogutils/flagutil.py
>>>> +++ b/mercurial/revlogutils/flagutil.py
>>>> @@ -78,3 +78,74 @@ def insertflagprocessor(flag, processor,
>>>>            msg = _("cannot register multiple processors on flag '%#x'.") % (flag)
>>>>            raise error.Abort(msg)
>>>>        flagprocessors[flag] = processor
>>>> +
>>>> +class flagprocessorsmixin(object):
>>>> +    """basic mixin to support revlog flag processing
>>>> +
>>>> +    Make sure the `_flagprocessors` attribute is set at ``__init__`` time.
>>>
>>> I feel a plain function taking (revlog, flagprocessors, ...) is simpler
>>> than the mixin class which implicitly depends on revlog attributes. But
>>> maybe I'm biased. I don't like class inheritance in general.
>>
>> Are you suggesting (revlog, currently_supported_flags, …)? If so, I am
>> afraid this would lead to wrong API usage.
> 
> Probably yes.
> 
>> It is important that the
>> revlog bear the supported flags to help enforcing extension/config
>> isolation.
> 
> Anyway, the user of the mixin would have to set self._flagprocessors
> properly. I don't see much difference between mixin + attributes and
> passing it as a function argument.

I am afraid people would simply call `(revlog, flagutils.flagprocessors, 
...)` instead of `(revlog, revlog._flagprocessors, ...)`.

Do you want me to switch to the function + attribute approach ?
Augie Fackler - Sept. 5, 2019, 6:35 p.m.
On Thu, Sep 05, 2019 at 08:43:16AM +0900, Yuya Nishihara wrote:
> On Mon, 2 Sep 2019 15:03:00 +0200, Pierre-Yves David wrote:
> > On 8/31/19 5:02 AM, Yuya Nishihara wrote:
> > > On Sun, 11 Aug 2019 13:10:47 +0200, Pierre-Yves David wrote:
> > >> # HG changeset patch
> > >> # User Pierre-Yves David <pierre-yves.david@octobus.net>
> > >> # Date 1565219568 -7200
> > >> #      Thu Aug 08 01:12:48 2019 +0200
> > >> # Node ID e6d22ec889461dbe819e0dbf4d2f0167736b2416
> > >> # Parent  6d61be152c5515fed315885f6c9cf9defe73de71
> > >> # EXP-Topic flag-processors
> > >> # Available At https://bitbucket.org/octobus/mercurial-devel/
> > >> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r e6d22ec88946
> > >> flagutil: introduce a flagprocessorsmixin class
> > >
> > > CC +indygreg
> > >
> > >> diff --git a/mercurial/revlogutils/flagutil.py b/mercurial/revlogutils/flagutil.py
> > >> --- a/mercurial/revlogutils/flagutil.py
> > >> +++ b/mercurial/revlogutils/flagutil.py
> > >> @@ -78,3 +78,74 @@ def insertflagprocessor(flag, processor,
> > >>           msg = _("cannot register multiple processors on flag '%#x'.") % (flag)
> > >>           raise error.Abort(msg)
> > >>       flagprocessors[flag] = processor
> > >> +
> > >> +class flagprocessorsmixin(object):
> > >> +    """basic mixin to support revlog flag processing
> > >> +
> > >> +    Make sure the `_flagprocessors` attribute is set at ``__init__`` time.
> > >
> > > I feel a plain function taking (revlog, flagprocessors, ...) is simpler
> > > than the mixin class which implicitly depends on revlog attributes. But
> > > maybe I'm biased. I don't like class inheritance in general.
> >
> > Are you suggesting (revlog, currently_supported_flags, …)? If so, I am
> > afraid this would lead to wrong API usage.
>
> Probably yes.
>
> > It is important that the
> > revlog bear the supported flags to help enforcing extension/config
> > isolation.
>
> Anyway, the user of the mixin would have to set self._flagprocessors
> properly. I don't see much difference between mixin + attributes and
> passing it as a function argument.
>
> > In all case, can we move forward with this series and adjust in a follow
> > up, I have a lot of other changes waiting on this ?
>
> If someone else agree with this change, please go ahead and queue this.
> I only have a _minor_ concern, and I don't have expertise around this code.

I'm also -0 on the mixin. Honestly I'm -0 on mixins in general,
especially with the direction I'd like to push remotefilelog and other
storage layers.

How does this interact with sqlitestore? It looks like as long as
self._flagprocessors is defined the right thing will happen, so could
we structure this to use composition rather than inheritance?

(If I get some decent answers on this I might queue it, but as it
stands this feels a little too much like a step backwards when it
comes to separation of concerns.)

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Augie Fackler - Sept. 5, 2019, 6:37 p.m.
On Thu, Sep 05, 2019 at 10:24:25AM +0200, Pierre-Yves David wrote:
>
>
> On 9/5/19 1:43 AM, Yuya Nishihara wrote:
> > On Mon, 2 Sep 2019 15:03:00 +0200, Pierre-Yves David wrote:
> > > On 8/31/19 5:02 AM, Yuya Nishihara wrote:
> > > > On Sun, 11 Aug 2019 13:10:47 +0200, Pierre-Yves David wrote:
> > > > > # HG changeset patch
> > > > > # User Pierre-Yves David <pierre-yves.david@octobus.net>
> > > > > # Date 1565219568 -7200
> > > > > #      Thu Aug 08 01:12:48 2019 +0200
> > > > > # Node ID e6d22ec889461dbe819e0dbf4d2f0167736b2416
> > > > > # Parent  6d61be152c5515fed315885f6c9cf9defe73de71
> > > > > # EXP-Topic flag-processors
> > > > > # Available At https://bitbucket.org/octobus/mercurial-devel/
> > > > > #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r e6d22ec88946
> > > > > flagutil: introduce a flagprocessorsmixin class
> > > >
> > > > CC +indygreg
> > > >
> > > > > diff --git a/mercurial/revlogutils/flagutil.py b/mercurial/revlogutils/flagutil.py
> > > > > --- a/mercurial/revlogutils/flagutil.py
> > > > > +++ b/mercurial/revlogutils/flagutil.py
> > > > > @@ -78,3 +78,74 @@ def insertflagprocessor(flag, processor,
> > > > >            msg = _("cannot register multiple processors on flag '%#x'.") % (flag)
> > > > >            raise error.Abort(msg)
> > > > >        flagprocessors[flag] = processor
> > > > > +
> > > > > +class flagprocessorsmixin(object):
> > > > > +    """basic mixin to support revlog flag processing
> > > > > +
> > > > > +    Make sure the `_flagprocessors` attribute is set at ``__init__`` time.
> > > >
> > > > I feel a plain function taking (revlog, flagprocessors, ...) is simpler
> > > > than the mixin class which implicitly depends on revlog attributes. But
> > > > maybe I'm biased. I don't like class inheritance in general.
> > >
> > > Are you suggesting (revlog, currently_supported_flags, …)? If so, I am
> > > afraid this would lead to wrong API usage.
> >
> > Probably yes.
> >
> > > It is important that the
> > > revlog bear the supported flags to help enforcing extension/config
> > > isolation.
> >
> > Anyway, the user of the mixin would have to set self._flagprocessors
> > properly. I don't see much difference between mixin + attributes and
> > passing it as a function argument.
>
> I am afraid people would simply call `(revlog, flagutils.flagprocessors,
> ...)` instead of `(revlog, revlog._flagprocessors, ...)`.
>
> Do you want me to switch to the function + attribute approach ?

I'd be more comfortable with that, yeah. Mention the "don't call this
with flagutils.flagprocesors" requirement in the docstring with the
rationale (IIRC it's because not all flag processors will be enabled
for all repos.)

>
>
> --
> Pierre-Yves David
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/revlog.py b/mercurial/revlog.py
--- a/mercurial/revlog.py
+++ b/mercurial/revlog.py
@@ -259,7 +259,7 @@  class revlogio(object):
             p = versionformat_pack(version) + p[4:]
         return p
 
-class revlog(object):
+class revlog(flagutil.flagprocessorsmixin):
     """
     the underlying revision storage object
 
@@ -1692,69 +1692,6 @@  class revlog(object):
         """
         return storageutil.hashrevisionsha1(text, p1, p2)
 
-    def _processflags(self, text, flags, operation, raw=False):
-        """Inspect revision data flags and applies transforms defined by
-        registered flag processors.
-
-        ``text`` - the revision data to process
-        ``flags`` - the revision flags
-        ``operation`` - the operation being performed (read or write)
-        ``raw`` - an optional argument describing if the raw transform should be
-        applied.
-
-        This method processes the flags in the order (or reverse order if
-        ``operation`` is 'write') defined by REVIDX_FLAGS_ORDER, applying the
-        flag processors registered for present flags. The order of flags defined
-        in REVIDX_FLAGS_ORDER needs to be stable to allow non-commutativity.
-
-        Returns a 2-tuple of ``(text, validatehash)`` where ``text`` is the
-        processed text and ``validatehash`` is a bool indicating whether the
-        returned text should be checked for hash integrity.
-
-        Note: If the ``raw`` argument is set, it has precedence over the
-        operation and will only update the value of ``validatehash``.
-        """
-        # fast path: no flag processors will run
-        if flags == 0:
-            return text, True
-        if not operation in ('read', 'write'):
-            raise error.ProgrammingError(_("invalid '%s' operation") %
-                                         operation)
-        # Check all flags are known.
-        if flags & ~flagutil.REVIDX_KNOWN_FLAGS:
-            raise error.RevlogError(_("incompatible revision flag '%#x'") %
-                                    (flags & ~flagutil.REVIDX_KNOWN_FLAGS))
-        validatehash = True
-        # Depending on the operation (read or write), the order might be
-        # reversed due to non-commutative transforms.
-        orderedflags = REVIDX_FLAGS_ORDER
-        if operation == 'write':
-            orderedflags = reversed(orderedflags)
-
-        for flag in orderedflags:
-            # If a flagprocessor has been registered for a known flag, apply the
-            # related operation transform and update result tuple.
-            if flag & flags:
-                vhash = True
-
-                if flag not in self._flagprocessors:
-                    message = _("missing processor for flag '%#x'") % (flag)
-                    raise error.RevlogError(message)
-
-                processor = self._flagprocessors[flag]
-                if processor is not None:
-                    readtransform, writetransform, rawtransform = processor
-
-                    if raw:
-                        vhash = rawtransform(self, text)
-                    elif operation == 'read':
-                        text, vhash = readtransform(self, text)
-                    else: # write operation
-                        text, vhash = writetransform(self, text)
-                validatehash = validatehash and vhash
-
-        return text, validatehash
-
     def checkhash(self, text, node, p1=None, p2=None, rev=None):
         """Check node hash integrity.
 
diff --git a/mercurial/revlogutils/flagutil.py b/mercurial/revlogutils/flagutil.py
--- a/mercurial/revlogutils/flagutil.py
+++ b/mercurial/revlogutils/flagutil.py
@@ -78,3 +78,74 @@  def insertflagprocessor(flag, processor,
         msg = _("cannot register multiple processors on flag '%#x'.") % (flag)
         raise error.Abort(msg)
     flagprocessors[flag] = processor
+
+class flagprocessorsmixin(object):
+    """basic mixin to support revlog flag processing
+
+    Make sure the `_flagprocessors` attribute is set at ``__init__`` time.
+
+    See the documentation of the ``_processflags`` method for details.
+    """
+
+    def _processflags(self, text, flags, operation, raw=False):
+        """Inspect revision data flags and applies transforms defined by
+        registered flag processors.
+
+        ``text`` - the revision data to process
+        ``flags`` - the revision flags
+        ``operation`` - the operation being performed (read or write)
+        ``raw`` - an optional argument describing if the raw transform should be
+        applied.
+
+        This method processes the flags in the order (or reverse order if
+        ``operation`` is 'write') defined by REVIDX_FLAGS_ORDER, applying the
+        flag processors registered for present flags. The order of flags defined
+        in REVIDX_FLAGS_ORDER needs to be stable to allow non-commutativity.
+
+        Returns a 2-tuple of ``(text, validatehash)`` where ``text`` is the
+        processed text and ``validatehash`` is a bool indicating whether the
+        returned text should be checked for hash integrity.
+
+        Note: If the ``raw`` argument is set, it has precedence over the
+        operation and will only update the value of ``validatehash``.
+        """
+        # fast path: no flag processors will run
+        if flags == 0:
+            return text, True
+        if not operation in ('read', 'write'):
+            raise error.ProgrammingError(_("invalid '%s' operation") %
+                                         operation)
+        # Check all flags are known.
+        if flags & ~REVIDX_KNOWN_FLAGS:
+            raise error.RevlogError(_("incompatible revision flag '%#x'") %
+                                    (flags & ~REVIDX_KNOWN_FLAGS))
+        validatehash = True
+        # Depending on the operation (read or write), the order might be
+        # reversed due to non-commutative transforms.
+        orderedflags = REVIDX_FLAGS_ORDER
+        if operation == 'write':
+            orderedflags = reversed(orderedflags)
+
+        for flag in orderedflags:
+            # If a flagprocessor has been registered for a known flag, apply the
+            # related operation transform and update result tuple.
+            if flag & flags:
+                vhash = True
+
+                if flag not in self._flagprocessors:
+                    message = _("missing processor for flag '%#x'") % (flag)
+                    raise error.RevlogError(message)
+
+                processor = self._flagprocessors[flag]
+                if processor is not None:
+                    readtransform, writetransform, rawtransform = processor
+
+                    if raw:
+                        vhash = rawtransform(self, text)
+                    elif operation == 'read':
+                        text, vhash = readtransform(self, text)
+                    else: # write operation
+                        text, vhash = writetransform(self, text)
+                validatehash = validatehash and vhash
+
+        return text, validatehash