This is an archive of the discontinued Mercurial Phabricator instance.

Differential D4292

pure: fix pure-Python revlog to support [0] lookups on empty log
AbandonedPublic

Authored by durin42 on Aug 15 2018, 11:02 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Summary

This fixes many many test failures in pure mode, but it appears to be a material behavior difference from the cext version. I tried adding this test:

def testindex0onemptyrevlog(self):
    want = (0, 0, 0, -1, -1, -1, -1, nullid)
    index, junk = parsers.parse_index2('', True)
    got = index[0]
    self.assertEqual(want, got)

to test-parseindex2.py, and it fails on native (but passes --pure
after this patch). I'm unclear what the correct behavior *should* be,
but hopefully this guides someone to a fix...

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

durin42 created this revision.Aug 15 2018, 11:02 PM
martinvonz added a comment.Aug 16 2018, 1:01 AM

I think the correct behavior should be what it is before this patch. I
think that matches the native code I think we should fix the callers
instead. There might be some callers in pure code that I forgot to update.
Can you check where it's called from with [0] on an empty log? Otherwise I
can look at it when I get back from vacation in a few days.

durin42 added a comment.Aug 16 2018, 1:13 AM
In D4292#66116, @martinvonz wrote:

I think the correct behavior should be what it is before this patch. I
think that matches the native code I think we should fix the callers
instead. There might be some callers in pure code that I forgot to update.
Can you check where it's called from with [0] on an empty log? Otherwise I
can look at it when I get back from vacation in a few days.

Okay, that's comforting in a way. :)

I can't make sense of the failures, so I'll probably just leave it for you (no pure code is obviously in the tb, so something else must be amiss somewhere. Ugh.)

martinvonz added a comment.Aug 18 2018, 2:58 AM
In D4292#66117, @durin42 wrote:
In D4292#66116, @martinvonz wrote:

I think the correct behavior should be what it is before this patch. I
think that matches the native code I think we should fix the callers
instead. There might be some callers in pure code that I forgot to update.
Can you check where it's called from with [0] on an empty log? Otherwise I
can look at it when I get back from vacation in a few days.

Okay, that's comforting in a way. :)
I can't make sense of the failures, so I'll probably just leave it for you (no pure code is obviously in the tb, so something else must be amiss somewhere. Ugh.)

Looks like Yuya fixed the problem in 13a1901176 (thanks!)

martinvonz added a comment.Aug 18 2018, 2:59 AM
In D4292#66487, @martinvonz wrote:
In D4292#66117, @durin42 wrote:
In D4292#66116, @martinvonz wrote:

I think the correct behavior should be what it is before this patch. I
think that matches the native code I think we should fix the callers
instead. There might be some callers in pure code that I forgot to update.
Can you check where it's called from with [0] on an empty log? Otherwise I
can look at it when I get back from vacation in a few days.

Okay, that's comforting in a way. :)
I can't make sense of the failures, so I'll probably just leave it for you (no pure code is obviously in the tb, so something else must be amiss somewhere. Ugh.)

Looks like Yuya fixed the problem in 13a1901176 (thanks!)

Sorry, mistaken copy, should have been 65d5de11.

yuja added a subscriber: yuja.Aug 18 2018, 4:09 AM

As a side note, prefix lookup of nullhex no longer works with pure since
null is excluded from the index.

martinvonz added a comment.Aug 19 2018, 2:33 AM
In D4292#66551, @yuja wrote:

As a side note, prefix lookup of nullhex no longer works with pure since
null is excluded from the index.

Yep, I had noticed. I have a fix that I'll send after running all pure and native tests on it will hopefully be the end of this series of patches fixing things I missed in a3dacabd476b.

durin42 abandoned this revision.Aug 30 2018, 5:24 PM

Revision Contents
Changeset List

PathPackages
Mmercurial/pure/parsers.py (2 lines)

Diff 10370

mercurial/pure/parsers.py

def _fix_index(self, i): def _fix_index(self, i):
if not isinstance(i, int): if not isinstance(i, int):
raise TypeError("expecting int indexes") raise TypeError("expecting int indexes")
if i < 0 or i >= len(self) + 1: if i < 0 or i >= len(self) + 1:
raise IndexError raise IndexError
return i return i
def __getitem__(self, i): def __getitem__(self, i):
if i == -1: if i == -1 or i == len(self) == 0:
return (0, 0, 0, -1, -1, -1, -1, nullid) return (0, 0, 0, -1, -1, -1, -1, nullid)
i = self._fix_index(i) i = self._fix_index(i)
if i >= self._lgt: if i >= self._lgt:
return self._extra[i - self._lgt] return self._extra[i - self._lgt]
index = self._calculate_index(i) index = self._calculate_index(i)
r = struct.unpack(indexformatng, self._data[index:index + indexsize]) r = struct.unpack(indexformatng, self._data[index:index + indexsize])
if i == 0: if i == 0:
e = list(r) e = list(r)