Page MenuHomePhabricator

changectx: add a "maybe filtered" filtered attribute
ClosedPublic

Authored by marmoute on Fri, Nov 22, 4:20 AM.

Details

Summary

There are changeset that we know not to be filtered (eg: null). In this case,
we could access some information without triggering changelog filtering. We add
an attribute to changectx that track this property.

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.Fri, Nov 22, 4:20 AM
indygreg added inline comments.
mercurial/context.py
509

Before I accept more of this series, I'd like others to think about the potential alternative solution of passing in or storing a reference to the changelog on basectx/changectx. The reason is that a lot of methods are calling repo.changelog and this can be expensive. I suspect we'd get a handful of random performance wins if we cached the changelog instance on each basectx. We also wouldn't need to litter the code with a bunch of conditional repo.unfiltered() calls since we'd already have an appropriate changelog instance stored in the instance.

Thoughts?

martinvonz added inline comments.
mercurial/context.py
484

Add a comment explaining what this attribute means, such as "indicates that this changeset is visible regardless of filtering". Actually, maybe it's better to name the property something like filter_agnostic?

509

That would make ctx.children() incorrect, right?

I still wonder if the "visible" filter should be removed and (so there would be no --hidden, and no annoying message telling you use that flag). We would need to figure out what e.g. hg log -r 'head()' and hg log -r x:: should do (to not include extinct heads) and how to make it easy for the user to get either behavior. Perhaps we would let head() be the visible heads and add a new allheads() to get all? That's obviously a much larger discussion and maybe a topic for the next sprint instead.

(grmlml, realising that I forgot to do the dual "submit" for my comment. It is 2019, why is Phabricator still not fixing this…)

mercurial/context.py
509

We woul still need the conditional instance since the maybe_filtered logic only apply to some of the changectx call. (eg: children need to go through filtering).

An overall better/more reobust solution to make the filtering lazier at the changelog level and to most the fastpath logic there. It would be more contained and more robust.

However, I decided I digged the rabbit hole enough for now and this other approach can be looked at later.

marmoute added inline comments.Sat, Nov 23, 12:01 PM
mercurial/context.py
484

I'll add a comment.

filter_agnostic is misleading. the revision might be filtered for another filter. Is is also borderline double negative.

509

The filtering is usefull at many level and removing it means dedicated code in many places (eg: discovery). That is error prone. I feel like effort would be better put to simply making it go fast (could be O(1) with proper indexing).
Which anoying message are you talking about ?

I agree that this is a larger discussion and I would rather not see the discussion around this series derails. +1 to have it independently later.

marmoute updated this revision to Diff 18362.Sat, Nov 23, 7:21 PM
indygreg accepted this revision.Sat, Nov 23, 7:44 PM

I'm satisfied with the discussion and will proceed with reviewing this series.

This revision is now accepted and ready to land.Sat, Nov 23, 7:44 PM
This revision was automatically updated to reflect the committed changes.