This is an archive of the discontinued Mercurial Phabricator instance.

revlog: deal with nodemap deletion within the index
ClosedPublic

Authored by marmoute on Nov 8 2019, 4:29 AM.

Details

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.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

marmoute created this revision.Nov 8 2019, 4:29 AM
marmoute updated this revision to Diff 17738.Nov 8 2019, 7:28 AM

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

mercurial/pure/parsers.py
60

pycompat.xrange.

mercurial/revlog.py
223

pycompat.xrange.

indygreg requested changes to this revision.Nov 8 2019, 2:41 PM
This revision now requires changes to proceed.Nov 8 2019, 2:41 PM
marmoute requested review of this revision.Nov 8 2019, 7:31 PM

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.

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.

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

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?

marmoute updated this revision to Diff 17834.Nov 9 2019, 12:15 AM

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.

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 ?)

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.