Page MenuHomePhabricator

ancestor: uniformity of calling lazyancestors classes
AbandonedPublic

Authored by gracinet on Dec 15 2018, 6:42 AM.

Details

Reviewers
indygreg
Group Reviewers
hg-reviewers
Summary

Up to now, the pure Python lazyancestors had been taking the parents
function in its constructor, whereas Rust-backed variants would have needed
a revlog index.

With this change, instantiation of all lazyancestors work uniformely with
an index. This requires only minor wrapping in test-ancestor.py.

It could be argued that this introduces a new duplication, and that
therefore, parentsfunc(index) should be provided in some utility module and
used both from revlog and ancestor modules.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

gracinet created this revision.Dec 15 2018, 6:42 AM

obviously, this one could be adapted for application before the rust-cpython bindings, and extended for the incrementalmissingancestors as well

yuja added a subscriber: yuja.Dec 15 2018, 11:46 PM

+def parentsfunc(index):
+ def parentrevs(rev):
+ try:
+ entry = index[rev]
+ except IndexError:
+ if rev == wdirrev:
+ raise error.WdirUnsupported
+ raise
+
+ return entry[5], entry[6]
+ return parentrevs

It doesn't seem correct that the ancestor module handles the revlog's
internals. Instead, can't we add a factory function which takes both
index and pfunc?

@yuja I'm not sure by what you consider exactly to be internals here. If that's the [5] and [6], maybe a parents(revision)` method on the index would be better ? The obvious drawback would be to write more C code.

Given that my ultimate goal here is to delegate the whole ancestors module to policy.importmod, where would we have the factory function you're thinking of ? In some sense, that's the role revlog.py fulfills isn't it ? Maybe all access to ancestors logic should keep on going through it ?

yuja added a comment.Dec 22 2018, 9:23 PM
@yuja I'm not sure by what you consider exactly to be internals here. If that's the `[5]` and [6]`, maybe a `parents(revision)` method on the index would be better ? The obvious drawback would be to write more C code.
Given that my ultimate goal here is to delegate the whole `ancestors` module to `policy.importmod`, where would we have the factory function you're thinking of ?

ancestor.lazyrevlogancestors(index, pfunc, revs, ...) -> lazyancestors

It allows us to select the implementation by the policy importer without
exposing the revlog internals too much. It doesn't look pretty, but IMHO
is less bad than moving revlog.parents() out of the revlog class.

In some sense, that's the role revlog.py fulfills isn't it ? Maybe all access to ancestors logic should keep on going through it ?

Strictly speaking, yes, it's the role of revlog.py. The pure ancestor.py
is agnostic on the graph implementation, whereas the Rust one isn't. So
I think we'll need some glue code for the time being.

gracinet abandoned this revision.Jan 5 2019, 6:46 AM

Given this conversation, and its outcome as materialised with latest versions of D5441 and D5442, we can now simply abandon this one.