D7321: revlog: deal with nodemap deletion within the index

2019-11-09 Thread marmoute (Pierre-Yves David) 
Closed by commit rHG5b556d46671d: revlog: deal with nodemap deletion within the 
index (authored by marmoute).
This revision was automatically updated to reflect the committed changes.
This revision was not accepted when it landed; it landed in state "Needs 
Review".

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D7321?vs=17834=17864

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

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

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

CHANGE DETAILS

diff --git a/mercurial/unionrepo.py b/mercurial/unionrepo.py
--- a/mercurial/unionrepo.py
+++ b/mercurial/unionrepo.py
@@ -83,7 +83,6 @@
 node,
 )
 self.index.append(e)
-self.nodemap[node] = n
 self.bundlerevs.add(n)
 n += 1
 
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
diff --git a/mercurial/bundlerepo.py b/mercurial/bundlerepo.py
--- a/mercurial/bundlerepo.py
+++ b/mercurial/bundlerepo.py
@@ -93,7 +93,6 @@
 node,
 )
 self.index.append(e)
-self.nodemap[node] = n
 self.bundlerevs.add(n)
 n += 1
 



To: marmoute, indygreg, #hg-reviewers
Cc: martinvonz, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D7321: revlog: deal with nodemap deletion within the index

2019-11-09 Thread marmoute (Pierre-Yves David) 
marmoute added a comment.


  In D7321#107872 , @martinvonz 
wrote:
  
  > In D7321#107839 , @marmoute 
wrote:
  >
  >> In 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 
, you wrote:
  >
  >> Without this change, 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  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 
 and 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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D7321: revlog: deal with nodemap deletion within the index

2019-11-08 Thread martinvonz (Martin von Zweigbergk) 
martinvonz added a comment.


  In D7321#107839 , @marmoute 
wrote:
  
  > In 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 
, you wrote:
  
  > Without this change, 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  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 
 and 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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D7321: revlog: deal with nodemap deletion within the index

2019-11-08 Thread marmoute (Pierre-Yves David) 
marmoute updated this revision to Diff 17834.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D7321?vs=17738=17834

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

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

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

CHANGE DETAILS

diff --git a/mercurial/unionrepo.py b/mercurial/unionrepo.py
--- a/mercurial/unionrepo.py
+++ b/mercurial/unionrepo.py
@@ -83,7 +83,6 @@
 node,
 )
 self.index.append(e)
-self.nodemap[node] = n
 self.bundlerevs.add(n)
 n += 1
 
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
diff --git a/mercurial/bundlerepo.py b/mercurial/bundlerepo.py
--- a/mercurial/bundlerepo.py
+++ b/mercurial/bundlerepo.py
@@ -93,7 +93,6 @@
 node,
 )
 self.index.append(e)
-self.nodemap[node] = n
 self.bundlerevs.add(n)
 n += 1
 



To: marmoute, indygreg, #hg-reviewers
Cc: martinvonz, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D7321: revlog: deal with nodemap deletion within the index

2019-11-08 Thread marmoute (Pierre-Yves David) 
marmoute added a comment.


  In 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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D7321: revlog: deal with nodemap deletion within the index

2019-11-08 Thread martinvonz (Martin von Zweigbergk) 
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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D7321: revlog: deal with nodemap deletion within the index

2019-11-08 Thread martinvonz (Martin von Zweigbergk) 
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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D7321: revlog: deal with nodemap deletion within the index

2019-11-08 Thread marmoute (Pierre-Yves David) 
marmoute added a comment.
marmoute requested review of this revision.


  regarding xrange: Same feedback as for 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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D7321: revlog: deal with nodemap deletion within the index

2019-11-08 Thread indygreg (Gregory Szorc) 
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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D7321: revlog: deal with nodemap deletion within the index

2019-11-08 Thread marmoute (Pierre-Yves David) 
marmoute updated this revision to Diff 17738.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D7321?vs=17737=17738

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

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

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

CHANGE DETAILS

diff --git a/mercurial/unionrepo.py b/mercurial/unionrepo.py
--- a/mercurial/unionrepo.py
+++ b/mercurial/unionrepo.py
@@ -83,7 +83,6 @@
 node,
 )
 self.index.append(e)
-self.nodemap[node] = n
 self.bundlerevs.add(n)
 n += 1
 
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
diff --git a/mercurial/bundlerepo.py b/mercurial/bundlerepo.py
--- a/mercurial/bundlerepo.py
+++ b/mercurial/bundlerepo.py
@@ -93,7 +93,6 @@
 node,
 )
 self.index.append(e)
-self.nodemap[node] = n
 self.bundlerevs.add(n)
 n += 1
 



To: marmoute, indygreg, #hg-reviewers
Cc: mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel