Patchwork [3,of,4,V2] changelog: prefilter in headrevs()

login
register
mail settings
Submitter Georges Racinet
Date Feb. 13, 2019, 11:47 a.m.
Message ID <eae29e210636ee44851e.1550058476@ishtar>
Download mbox | patch
Permalink /patch/38702/
State Accepted
Headers show

Comments

Georges Racinet - Feb. 13, 2019, 11:47 a.m.
# HG changeset patch
# User Georges Racinet <georges.racinet@octobus.net>
# Date 1547815081 -3600
#      Fri Jan 18 13:38:01 2019 +0100
# Node ID eae29e210636ee44851e0caa385097a090c60af8
# Parent  956c5b54b4ce3e8decf5243a7f73c6f9a06f1229
# EXP-Topic revset.predicates
changelog: prefilter in headrevs()

In case where headrevs() is called on some revisions, we perform
the check that aren't filtered in advance, and switch revlog to
use its unchecked form.

This allows to work with alternative implementations that don't have knowledge
of the filtering system, such as the Rust one.
Yuya Nishihara - Feb. 13, 2019, 1:18 p.m.
On Wed, 13 Feb 2019 12:47:56 +0100, Georges Racinet wrote:
> # HG changeset patch
> # User Georges Racinet <georges.racinet@octobus.net>
> # Date 1547815081 -3600
> #      Fri Jan 18 13:38:01 2019 +0100
> # Node ID eae29e210636ee44851e0caa385097a090c60af8
> # Parent  956c5b54b4ce3e8decf5243a7f73c6f9a06f1229
> # EXP-Topic revset.predicates
> changelog: prefilter in headrevs()

Queued the first two patches, thanks.

> +    def _checknofilteredgenrevs(self, revs):
> +        """rewrap 'revs' generator to include check for filtered revisions
> +
> +        This does not consume the incoming generator.
> +        """
> +        filteredrevs = self.filteredrevs
> +        for r in revs:
> +            if r in filteredrevs:
> +                raise error.FilteredIndexError(r)
> +            yield r
> +
> +    def _checknofilteredinrevs(self, revs):
> +        """raise the appropriate error if 'revs' contains a filtered revision
> +
> +        This returns a version of 'revs' to be used by the caller, that works
> +        for all cases, including lazy ones
> +        """
> +        safehasattr = util.safehasattr
> +        if safehasattr(revs, '__next__'):
> +            # Note that inspect.isgenerator() is not true for iterators,
> +            # and that calling even implicitely iter() on a iterator does not
> +            # clone it
> +            return self._checknofilteredgenrevs(revs)
> +
> +        filteredrevs = self.filteredrevs
> +        if safehasattr(revs, 'first'):  # smartset
> +            offenders = revs & filteredrevs
> +        else:
> +            offenders = filteredrevs.intersection(revs)
> +
> +        for rev in offenders:
> +            raise error.FilteredIndexError(rev)
> +        return revs

Do we need this complexity to handle various types of 'revs' differently?

IIUC, revlog.headrevs(revs) is just forwarded to dagop.headrevs(revs), where
'revs' is first converted to a set, and then iterated. Which means 'revs'
can't be an iterator, and the order doesn't basically matter.
Georges Racinet - Feb. 14, 2019, 1:53 p.m.
On 2/13/19 2:18 PM, Yuya Nishihara wrote:
> On Wed, 13 Feb 2019 12:47:56 +0100, Georges Racinet wrote:
>> # HG changeset patch
>> # User Georges Racinet <georges.racinet@octobus.net>
>> # Date 1547815081 -3600
>> #      Fri Jan 18 13:38:01 2019 +0100
>> # Node ID eae29e210636ee44851e0caa385097a090c60af8
>> # Parent  956c5b54b4ce3e8decf5243a7f73c6f9a06f1229
>> # EXP-Topic revset.predicates
>> changelog: prefilter in headrevs()
> Queued the first two patches, thanks.
>
>> +    def _checknofilteredgenrevs(self, revs):
>> +        """rewrap 'revs' generator to include check for filtered revisions
>> +
>> +        This does not consume the incoming generator.
>> +        """
>> +        filteredrevs = self.filteredrevs
>> +        for r in revs:
>> +            if r in filteredrevs:
>> +                raise error.FilteredIndexError(r)
>> +            yield r
>> +
>> +    def _checknofilteredinrevs(self, revs):
>> +        """raise the appropriate error if 'revs' contains a filtered revision
>> +
>> +        This returns a version of 'revs' to be used by the caller, that works
>> +        for all cases, including lazy ones
>> +        """
>> +        safehasattr = util.safehasattr
>> +        if safehasattr(revs, '__next__'):
>> +            # Note that inspect.isgenerator() is not true for iterators,
>> +            # and that calling even implicitely iter() on a iterator does not
>> +            # clone it
>> +            return self._checknofilteredgenrevs(revs)
>> +
>> +        filteredrevs = self.filteredrevs
>> +        if safehasattr(revs, 'first'):  # smartset
>> +            offenders = revs & filteredrevs
>> +        else:
>> +            offenders = filteredrevs.intersection(revs)
>> +
>> +        for rev in offenders:
>> +            raise error.FilteredIndexError(rev)
>> +        return revs
> Do we need this complexity to handle various types of 'revs' differently?
>
> IIUC, revlog.headrevs(revs) is just forwarded to dagop.headrevs(revs), where
> 'revs' is first converted to a set, and then iterated. Which means 'revs'
> can't be an iterator, and the order doesn't basically matter.

So to rephrase you're saying that no caller will ever expect an iterator
not to be consumed by calling headrevs() on it, due to filtering,
because that's what happens anyway, even without any filtering. That's
right indeed, I've been overprotective on that one.

I'll make a v3 for this patch and its successor, thanks for the review
and the queueing !

Note: I'll be offline until the 20th, not sure I can make the v3 before
that.

Patch

diff -r 956c5b54b4ce -r eae29e210636 mercurial/changelog.py
--- a/mercurial/changelog.py	Thu Jan 10 18:25:18 2019 +0100
+++ b/mercurial/changelog.py	Fri Jan 18 13:38:01 2019 +0100
@@ -22,6 +22,7 @@ 
     error,
     pycompat,
     revlog,
+    util,
 )
 from .utils import (
     dateutil,
@@ -350,6 +351,40 @@ 
     def reachableroots(self, minroot, heads, roots, includepath=False):
         return self.index.reachableroots2(minroot, heads, roots, includepath)
 
+    def _checknofilteredgenrevs(self, revs):
+        """rewrap 'revs' generator to include check for filtered revisions
+
+        This does not consume the incoming generator.
+        """
+        filteredrevs = self.filteredrevs
+        for r in revs:
+            if r in filteredrevs:
+                raise error.FilteredIndexError(r)
+            yield r
+
+    def _checknofilteredinrevs(self, revs):
+        """raise the appropriate error if 'revs' contains a filtered revision
+
+        This returns a version of 'revs' to be used by the caller, that works
+        for all cases, including lazy ones
+        """
+        safehasattr = util.safehasattr
+        if safehasattr(revs, '__next__'):
+            # Note that inspect.isgenerator() is not true for iterators,
+            # and that calling even implicitely iter() on a iterator does not
+            # clone it
+            return self._checknofilteredgenrevs(revs)
+
+        filteredrevs = self.filteredrevs
+        if safehasattr(revs, 'first'):  # smartset
+            offenders = revs & filteredrevs
+        else:
+            offenders = filteredrevs.intersection(revs)
+
+        for rev in offenders:
+            raise error.FilteredIndexError(rev)
+        return revs
+
     def headrevs(self, revs=None):
         if revs is None and self.filteredrevs:
             try:
@@ -359,6 +394,8 @@ 
             except AttributeError:
                 return self._headrevs()
 
+        if self.filteredrevs:
+            revs = self._checknofilteredinrevs(revs)
         return super(changelog, self).headrevs(revs)
 
     def strip(self, *args, **kwargs):
diff -r 956c5b54b4ce -r eae29e210636 mercurial/revlog.py
--- a/mercurial/revlog.py	Thu Jan 10 18:25:18 2019 +0100
+++ b/mercurial/revlog.py	Fri Jan 18 13:38:01 2019 +0100
@@ -1121,7 +1121,7 @@ 
                 return self.index.headrevs()
             except AttributeError:
                 return self._headrevs()
-        return dagop.headrevs(revs, self.parentrevs)
+        return dagop.headrevs(revs, self._uncheckedparentrevs)
 
     def computephases(self, roots):
         return self.index.computephasesmapsets(roots)