Page MenuHomePhabricator

branchmap: refactor revbranchmap and use it as topicmap [PoC]
Needs RevisionPublic

Authored by joerg.sonnenberger on Dec 14 2020, 4:38 PM.

Details

Reviewers
baymax
Group Reviewers
hg-reviewers

Diff Detail

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

Event Timeline

pulkit added a subscriber: pulkit.Dec 17 2020, 6:03 AM

While it's still POC, having some commit message on how we are tackling the problem will be helpful.

The proof of concept part is primarily that we can get the benefit with minimal refactoring of the existing rbc implementation. The actual use is a bit more complicated as the actual topicmap would be part of an extension and therefore hook up differently. But the interaction is much easier to understand like this, IMO.

I like where this is headed. Thank you for doing this work. I can see this patch can be split into at least 3 different patches which will help speed up review.

mercurial/branchmap.py
547

This movement of variables can be a separate patch and will be easy one to queue making this patch a bit lighter to review.

782

Probably we can have an interface for generic name-cache.

mercurial/changelog.py
599

This and related changes should be a separate patch.

I like where this is headed. Thank you for doing this work. I can see this patch can be split into at least 3 different patches which will help speed up review.

... of course when it's not a POC anymore.

mercurial/branchmap.py
599

Also, what does a caller passing extra as None is expecting here? It will be nice to not have this an optional argument.

We can also pass a rev/node here and do changelog.changelogrevision thing in this function.

mercurial/branchmap.py
547

Right, the primary goal of this PoC is to outline what changes are necessary and what the impact is for supporting more than just the branch mapping. It doesn't make sense as is since it is provided for the benefit out of currently out-of-tree target. So the goal is to get a consensus on whether the cache is useful enough for the topic extension and if it is, split this patch into smaller pieces, figure out a few details in the method naming (e.g. branchinfo is tight to the current functionality) in core and how to integrate the rest backwards compatible in the topic extension.

599

As mentioned in the review for the parent, doing a lookup here is expensive and should be avoided. The extra=None case is essentially only used for nullrev. The function name is not terribly good, parsechangeset or so would make a lot more sense in the generalized cache.

mercurial/changelog.py
599

Yes, but it is a breaking change for topic.

baymax requested changes to this revision.May 25 2021, 5:32 AM

There seems to have been no activities on this Diff for the past 3 Months.

By policy, we are automatically moving it out of the need-review state.

Please, move it back to need-review without hesitation if this diff should still be discussed.

:baymax:need-review-idle:

This revision now requires changes to proceed.May 25 2021, 5:32 AM