Patchwork D7321: revlog: deal with nodemap deletion within the index

login
register
mail settings
Submitter phabricator
Date Nov. 8, 2019, 9:32 a.m.
Message ID <differential-rev-PHID-DREV-erqk7ag2q75rc7eytcre-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/42918/
State Superseded
Headers show

Comments

phabricator - Nov. 8, 2019, 9:32 a.m.
marmoute created this revision.
Herald added a reviewer: indygreg.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Since the nodemap data now live in the index, it should be the index
  responsibility to ensure the data are up to date.
  
  The C version of the index is already dealing with such deletion.
  
  This work is part of a refactoring to unify the revlog index and the nodemap.
  This unification prepare the use of a persistent nodemap.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/pure/parsers.py
  mercurial/revlog.py

CHANGE DETAILS




To: marmoute, indygreg, #hg-reviewers
Cc: mercurial-devel
phabricator - Nov. 8, 2019, 7:41 p.m.
indygreg added a comment.


  I approve of the direction. In addition to inline comments, same deal as last patch: is there a sentinel entry to worry about?

INLINE COMMENTS

> parsers.py:60
> +        if 'nodemap' in vars(self):
> +            for r in range(start, len(self)):
> +                n = self[r][7]

`pycompat.xrange`.

> revlog.py:223
> +            raise ValueError(b"deleting slices only supports a:-1 with step 1")
> +        for r in pycompat.xrange(i.start, len(self)):
> +            del self.nodemap[self[r][7]]

`pycompat.xrange`.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7321/new/

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

To: marmoute, indygreg, #hg-reviewers
Cc: mercurial-devel
phabricator - Nov. 9, 2019, 12:31 a.m.
marmoute added a comment.
marmoute requested review of this revision.


  regarding xrange: Same feedback as for D7313 <https://phab.mercurial-scm.org/D7313>, this is a non performance critical section since this is about `pure`.
  
  regarding the sentinel value: The revision we access during deletion are the same as the one actually removed from the index, so this seems correct. In addition, all test are happy.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7321/new/

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

To: marmoute, indygreg, #hg-reviewers
Cc: mercurial-devel
phabricator - Nov. 9, 2019, 1:27 a.m.
martinvonz added a comment.


  I think our comments up to here have been addressed (I'm pretty confident that the `start + 1` thing is correct), so I'll queue this part. The only thing left on the rest of the series is the module policy thing I just sent a comment about on the next patch.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7321/new/

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

To: marmoute, indygreg, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Nov. 9, 2019, 1:52 a.m.
martinvonz added a comment.


  I get a traceback from `test-copytrace-heuristics.t` with this patch.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7321/new/

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

To: marmoute, indygreg, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Nov. 9, 2019, 5:06 a.m.
marmoute added a comment.


  In D7321#107805 <https://phab.mercurial-scm.org/D7321#107805>, @martinvonz wrote:
  
  > I get a traceback from `test-copytrace-heuristics.t` with this patch.
  
  Well… I don't. Can you share some details? How are you running your tests?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7321/new/

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

To: marmoute, indygreg, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Nov. 9, 2019, 6:58 a.m.
martinvonz added a comment.


  In D7321#107839 <https://phab.mercurial-scm.org/D7321#107839>, @marmoute wrote:
  
  > In D7321#107805 <https://phab.mercurial-scm.org/D7321#107805>, @martinvonz wrote:
  >
  >> I get a traceback from `test-copytrace-heuristics.t` with this patch.
  >
  > Well… I don't. Can you share some details? How are you running your tests?
  
  I think I figured it out. And so did you, I think. We just didn't know that were talking about the same failure :) On D7320 <https://phab.mercurial-scm.org/D7320>, you wrote:
  
  > Without this change, D7321 <https://phab.mercurial-scm.org/D7321> crash horribly, and debugging show that the value left out is clearly a non-sentinel value that should be deleted.
  > With this change, D7321 <https://phab.mercurial-scm.org/D7321> pass all test flawlessly.
  
  I think the "crash horribly" there was referring to `test-copytrace-heuristics.t`. If I don't recompile after applying D7320 <https://phab.mercurial-scm.org/D7320> and D7321 <https://phab.mercurial-scm.org/D7321> (this patch), then I get a crash ending like this:
  
    +    File "/usr/local/google/home/martinvonz/hg/mercurial/bundle2.py", line 489, in _processchangegroup
    +      ret = cg.apply(op.repo, tr, source, url, **kwargs)
    +    File "/usr/local/google/home/martinvonz/hg/mercurial/changegroup.py", line 345, in apply
    +      self._unpackmanifests(repo, revmap, trp, progress)
    +    File "/usr/local/google/home/martinvonz/hg/mercurial/changegroup.py", line 258, in _unpackmanifests
    +      repo.manifestlog.getstorage(b'').addgroup(deltas, revmap, trp)
    +    File "/usr/local/google/home/martinvonz/hg/mercurial/manifest.py", line 1762, in addgroup
    +      deltas, linkmapper, transaction, addrevisioncb=addrevisioncb
    +    File "/usr/local/google/home/martinvonz/hg/mercurial/revlog.py", line 2305, in addgroup
    +      if node in self.nodemap:
    +  IndexError: could not access rev 3
    +  [1]
  
  I assume that's what you also saw, but please confirm.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7321/new/

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

To: marmoute, indygreg, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Nov. 9, 2019, 8:07 a.m.
marmoute added a comment.


  In D7321#107872 <https://phab.mercurial-scm.org/D7321#107872>, @martinvonz wrote:
  
  > In D7321#107839 <https://phab.mercurial-scm.org/D7321#107839>, @marmoute wrote:
  >
  >> In D7321#107805 <https://phab.mercurial-scm.org/D7321#107805>, @martinvonz wrote:
  >>
  >>> I get a traceback from `test-copytrace-heuristics.t` with this patch.
  >>
  >> Well… I don't. Can you share some details? How are you running your tests?
  >
  > I think I figured it out. And so did you, I think. We just didn't know that were talking about the same failure :) On D7320 <https://phab.mercurial-scm.org/D7320>, you wrote:
  >
  >> Without this change, D7321 <https://phab.mercurial-scm.org/D7321> crash horribly, and debugging show that the value left out is clearly a non-sentinel value that should be deleted.
  >> With this change, D7321 <https://phab.mercurial-scm.org/D7321> pass all test flawlessly.
  >
  > I think the "crash horribly" there was referring to `test-copytrace-heuristics.t`. If I don't recompile after applying D7320 <https://phab.mercurial-scm.org/D7320> and D7321 <https://phab.mercurial-scm.org/D7321> (this patch), then I get a crash ending like this:
  >
  >   +    File "/usr/local/google/home/martinvonz/hg/mercurial/bundle2.py", line 489, in _processchangegroup
  >   +      ret = cg.apply(op.repo, tr, source, url, **kwargs)
  >   +    File "/usr/local/google/home/martinvonz/hg/mercurial/changegroup.py", line 345, in apply
  >   +      self._unpackmanifests(repo, revmap, trp, progress)
  >   +    File "/usr/local/google/home/martinvonz/hg/mercurial/changegroup.py", line 258, in _unpackmanifests
  >   +      repo.manifestlog.getstorage(b'').addgroup(deltas, revmap, trp)
  >   +    File "/usr/local/google/home/martinvonz/hg/mercurial/manifest.py", line 1762, in addgroup
  >   +      deltas, linkmapper, transaction, addrevisioncb=addrevisioncb
  >   +    File "/usr/local/google/home/martinvonz/hg/mercurial/revlog.py", line 2305, in addgroup
  >   +      if node in self.nodemap:
  >   +  IndexError: could not access rev 3
  >   +  [1]
  >
  > I assume that's what you also saw, but please confirm.
  
  Yeah, this is a lack of recompilation of the C code. (maybe I should bump the version at very single patch touching the c file ?)

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7321/new/

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

To: marmoute, indygreg, #hg-reviewers
Cc: martinvonz, mercurial-devel

Patch

diff --git a/mercurial/revlog.py b/mercurial/revlog.py
--- a/mercurial/revlog.py
+++ b/mercurial/revlog.py
@@ -217,6 +217,13 @@ 
         self.nodemap[tup[7]] = len(self)
         super(revlogoldindex, self).append(tup)
 
+    def __delitem__(self, i):
+        if not isinstance(i, slice) or not i.stop == -1 or i.step is not None:
+            raise ValueError(b"deleting slices only supports a:-1 with step 1")
+        for r in pycompat.xrange(i.start, len(self)):
+            del self.nodemap[self[r][7]]
+        super(revlogoldindex, self).__delitem__(i)
+
     def clearcaches(self):
         self.__dict__.pop('nodemap', None)
 
@@ -2431,8 +2438,6 @@ 
         self._revisioncache = None
         self._chaininfocache = {}
         self._chunkclear()
-        for x in pycompat.xrange(rev, len(self)):
-            del self.nodemap[self.node(x)]
 
         del self.index[rev:-1]
         self._nodepos = None
diff --git a/mercurial/pure/parsers.py b/mercurial/pure/parsers.py
--- a/mercurial/pure/parsers.py
+++ b/mercurial/pure/parsers.py
@@ -55,6 +55,12 @@ 
             nodemap[n] = r
         return nodemap
 
+    def _stripnodes(self, start):
+        if 'nodemap' in vars(self):
+            for r in range(start, len(self)):
+                n = self[r][7]
+                del self.nodemap[n]
+
     def clearcaches(self):
         self.__dict__.pop('nodemap', None)
 
@@ -103,6 +109,7 @@ 
             raise ValueError(b"deleting slices only supports a:-1 with step 1")
         i = i.start
         self._check_index(i)
+        self._stripnodes(i)
         if i < self._lgt:
             self._data = self._data[: i * indexsize]
             self._lgt = i
@@ -140,6 +147,7 @@ 
             raise ValueError(b"deleting slices only supports a:-1 with step 1")
         i = i.start
         self._check_index(i)
+        self._stripnodes(i)
         if i < self._lgt:
             self._offsets = self._offsets[:i]
             self._lgt = i