This is an archive of the discontinued Mercurial Phabricator instance.

cext-revlog: fixed __delitem__ for uninitialized nodetree
ClosedPublic

Authored by gracinet on Dec 11 2019, 6:52 AM.

Details

Summary

This is a bug in a code path that's seldom used, because in practice
(at least in the whole test suite), calls to del index[i:j] currently
just don't happen before the nodetree has been initialized.
However, in our current work to replace the nodetree by a Rust implementation,
this is of course systematic.

In index_slice_del(), if the slice start is smaller than self->length,
the whole of self->added has to be cleared.

Before this change, the clearing was done only by the call to
index_invalidate_added(self, 0), that happens only for initialized
nodetrees. Hence the removal was effective only from start to self->length.

The consequence is index corruption, with bogus results in subsequent calls,
and in particular errors such as ValueError("parent out of range"), due to
the fact that parents of entries in self->added are now just invalid.

This is detected by the rebase tests, under conditions that the nodetree
of revlog.c is never initialized. The provided specific test is more direct.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

gracinet created this revision.Dec 11 2019, 6:52 AM
This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.

This broke ./run-tests.py --pure test-parseindex2.py

oops, sorry, had no idea the exact same test was running for the pure imp (although it makes sense). I'll check into it, it's not obvious at first sight why it should not work.

Here's a quickfix that works for me for both pure and C

-- a/tests/test-parseindex2.py Thu Dec 05 20:41:23 2019 +0100
+++ b/tests/test-parseindex2.py Thu Dec 12 18:34:28 2019 +0100
@@ -18,6 +18,7 @@
     node as nodemod,
     policy,
     pycompat,
+    util,
 )
 
 parsers = policy.importmod('parsers')
@@ -267,11 +268,12 @@
         appendrev(6)
         self.assertEqual(len(index), 7)
 
-        del index[1:7]
+        del index[1:-1]
 
         # assertions that failed before correction
         self.assertEqual(len(index), 1)  # was 4
-        self.assertEqual(index.headrevs(), [0])  # gave ValueError
+        if util.safehasattr(index, 'headrevs'):  # not implemented in pure
+            self.assertEqual(index.headrevs(), [0])  # gave ValueError

Explanation: the C version would allow either -1 or actual length, the pure version just allows -1. The pure version does not have algorithms such as headrevs, they are implemented in revlog.py

I can redo that cleanly, just tell me the expected way.

Here's a quickfix that works for me for both pure and C

-- a/tests/test-parseindex2.py Thu Dec 05 20:41:23 2019 +0100
+++ b/tests/test-parseindex2.py Thu Dec 12 18:34:28 2019 +0100
@@ -18,6 +18,7 @@
     node as nodemod,
     policy,
     pycompat,
+    util,
 )
 parsers = policy.importmod('parsers')
@@ -267,11 +268,12 @@
         appendrev(6)
         self.assertEqual(len(index), 7)
-        del index[1:7]
+        del index[1:-1]
         # assertions that failed before correction
         self.assertEqual(len(index), 1)  # was 4
-        self.assertEqual(index.headrevs(), [0])  # gave ValueError
+        if util.safehasattr(index, 'headrevs'):  # not implemented in pure
+            self.assertEqual(index.headrevs(), [0])  # gave ValueError

Explanation: the C version would allow either -1 or actual length, the pure version just allows -1. The pure version does not have algorithms such as headrevs, they are implemented in revlog.py
I can redo that cleanly, just tell me the expected way.

Seems fine to just send that as a regular patch. It's too late to amend this one now. Thanks.