Patchwork D6207: branchcache: add functions to validate changelog nodes

login
register
mail settings
Submitter phabricator
Date April 5, 2019, 1:31 p.m.
Message ID <differential-rev-PHID-DREV-5sj2dirqo3wk3y3jcwap-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/39504/
State Superseded
Headers show

Comments

phabricator - April 5, 2019, 1:31 p.m.
pulkit created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This patch adds functions to validate closed nodes, validate nodes for a certain
  branch and for all the branches. These functions will be used in upcoming
  patches.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6207

AFFECTED FILES
  mercurial/branchmap.py

CHANGE DETAILS




To: pulkit, #hg-reviewers
Cc: mercurial-devel
Yuya Nishihara - April 15, 2019, 11:14 p.m.
Queued, thanks.

> +    def _verifybranch(self, branch):
> +        """ verify head nodes for the given branch. If branch is None, verify
> +        for all the branches """

"If branch is None, ..." appears wrong.

> +        if branch not in self._entries or branch in self._verifiedbranches:
> +            return
> +        for n in self._entries[branch]:
> +            if not self._hasnode(n):
> +                _unknownnode(n)
> +
> +        self._verifiedbranches.add(branch)

Regarding D6236, `_verifiedbranches` could be inverted (i.e. a set of branches
to be verified) so that `_verifyall()` can return early. I don't know which
will be faster, but in principle, fewer loops and Python gives a better result.
phabricator - April 15, 2019, 11:16 p.m.
yuja added a comment.


  Queued, thanks.
  
  > +    def _verifybranch(self, branch):
  >  +        """ verify head nodes for the given branch. If branch is None, verify
  >  +        for all the branches """
  
  "If branch is None, ..." appears wrong.
  
  > +        if branch not in self._entries or branch in self._verifiedbranches:
  >  +            return
  >  +        for n in self._entries[branch]:
  >  +            if not self._hasnode(n):
  >  +                _unknownnode(n)
  >  +
  >  +        self._verifiedbranches.add(branch)
  
  Regarding https://phab.mercurial-scm.org/D6236, `_verifiedbranches` could be inverted (i.e. a set of branches
  to be verified) so that `_verifyall()` can return early. I don't know which
  will be faster, but in principle, fewer loops and Python gives a better result.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6207

To: pulkit, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - April 16, 2019, 12:35 p.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/D6207#90778, @yuja wrote:
  
  > Queued, thanks.
  >
  > > +    def _verifybranch(self, branch):
  > >  +        """ verify head nodes for the given branch. If branch is None, verify
  > >  +        for all the branches """
  >
  > "If branch is None, ..." appears wrong.
  >
  > > +        if branch not in self._entries or branch in self._verifiedbranches:
  > >  +            return
  > >  +        for n in self._entries[branch]:
  > >  +            if not self._hasnode(n):
  > >  +                _unknownnode(n)
  > >  +
  > >  +        self._verifiedbranches.add(branch)
  >
  > Regarding https://phab.mercurial-scm.org/D6236, `_verifiedbranches` could be inverted (i.e. a set of branches
  >  to be verified) so that `_verifyall()` can return early. I don't know which
  >  will be faster, but in principle, fewer loops and Python gives a better result.
  
  
  In _verifyall(), I changed code to calculate a needverification set and then iterate over it.
  
  The motivation behind iteritems() change (https://phab.mercurial-scm.org/D6236) is optimizing https://www.mercurial-scm.org/repo/hg-committed/file/70b71421fd33/mercurial/commands.py#l1128.
  
  Maybe we can iterate over verified nodes first, and then iterate over unverified nodes?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6207

To: pulkit, #hg-reviewers
Cc: yuja, mercurial-devel
Yuya Nishihara - April 16, 2019, 10:46 p.m.
>   The motivation behind iteritems() change (https://phab.mercurial-scm.org/D6236) is optimizing https://www.mercurial-scm.org/repo/hg-committed/file/70b71421fd33/mercurial/commands.py#l1128.

Okay, got that, thanks.

>   Maybe we can iterate over verified nodes first, and then iterate over unverified nodes?

Maybe no. I don't think it's more likely for callers to get any verified node
and stop the iteration.
phabricator - April 16, 2019, 11:08 p.m.
yuja added a comment.


  >   The motivation behind iteritems() change (https://phab.mercurial-scm.org/D6236) is optimizing https://www.mercurial-scm.org/repo/hg-committed/file/70b71421fd33/mercurial/commands.py#l1128.
  
  Okay, got that, thanks.
  
  >   Maybe we can iterate over verified nodes first, and then iterate over unverified nodes?
  
  Maybe no. I don't think it's more likely for callers to get any verified node
  and stop the iteration.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6207

To: pulkit, #hg-reviewers
Cc: yuja, mercurial-devel

Patch

diff --git a/mercurial/branchmap.py b/mercurial/branchmap.py
--- a/mercurial/branchmap.py
+++ b/mercurial/branchmap.py
@@ -126,6 +126,10 @@ 
     def clear(self):
         self._per_filter.clear()
 
+def _unknownnode(node):
+    """ raises ValueError when branchcache found a node which does not exists
+    """
+    raise ValueError(r'node %s does not exist' % pycompat.sysstr(hex(node)))
 
 class branchcache(object):
     """A dict like object that hold branches heads cache.
@@ -173,6 +177,32 @@ 
         if self._hasnode is None:
             self._hasnode = lambda x: True
 
+    def _verifyclosed(self):
+        """ verify the closed nodes we have """
+        if self._closedverified:
+            return
+        for node in self._closednodes:
+            if not self._hasnode(node):
+                _unknownnode(node)
+
+        self._closedverified = True
+
+    def _verifybranch(self, branch):
+        """ verify head nodes for the given branch. If branch is None, verify
+        for all the branches """
+        if branch not in self._entries or branch in self._verifiedbranches:
+            return
+        for n in self._entries[branch]:
+            if not self._hasnode(n):
+                _unknownnode(n)
+
+        self._verifiedbranches.add(branch)
+
+    def _verifyall(self):
+        """ verifies nodes of all the branches """
+        for b in self._entries:
+            self._verifybranch(b)
+
     def __iter__(self):
         return iter(self._entries)