This is an archive of the discontinued Mercurial Phabricator instance.

repoview: add a new attribute _visibilityexceptions and related API
AbandonedPublic

Authored by pulkit on Nov 2 2017, 2:04 PM.

Details

Reviewers
quark
yuja
Group Reviewers
hg-reviewers
Summary

Currently we don't have a defined way in core to make some hidden revisions
visible in filtered repo. Extensions to achieve the purpose of unhiding some
hidden commits, wrap repoview.pinnedrevs() function.

To make the above task simple and have well defined API, this patch adds a new
attribute '_visibilityexceptions' to repoview class which will contains
the hidden revs which should be exception.
This will allow to set different exceptions for different repoview objects
backed by the same unfiltered repo.

This patch also adds API to add revs to the attribute set and get them.

Thanks to Jun for suggesting the use of repoview class instead of localrepo.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

pulkit created this revision.Nov 2 2017, 2:04 PM
quark added a subscriber: quark.EditedNov 9 2017, 3:54 PM

I think localrepo object is usually for unfiltered access therefore the visibility exception API looks strange to me.

Maybe move them to the repoview layer?

mercurial/localrepo.py
519

nit: do not need a space around """

In D1285#22442, @quark wrote:

I think localrepo object is usually for unfiltered access therefore the visibility exception API looks strange to me.
Maybe move them to the repoview layer?

The functions to compute filtered set takes unfiltered repository, and we consider these changesets at that time, so unfiltered make sense.
Also, having them at repoview layer will require a filtered repository to access the API's. This will make the use of API a bit difficult as it may throw an AttributeError if we are working with unfiltered repo or the user of the API has to know which filtername to use etc.

quark added a comment.EditedNov 20 2017, 4:20 PM

I mean, having an empty shim in unfiltered repo:

def addvisibilityexceptions(...):
    pass

def getvisibilityexceptions(...):
    return ()

I think the state of "visibilityexception" should really be bounded into whoever controls "filteredrevs", which is the repoview object. This allows different exceptions to be set for different "repoview" objects backed by a same unfiltered repo, which seems more desirable.

pulkit edited the summary of this revision. (Show Details)Nov 22 2017, 8:23 PM
pulkit retitled this revision from localrepo: add a new attribute _visibilityexceptions and related API to repoview: add a new attribute _visibilityexceptions and related API.
pulkit updated this revision to Diff 3784.
In D1285#24461, @quark wrote:

I think the state of "visibilityexception" should really be bounded into whoever controls "filteredrevs", which is the repoview object. This allows different exceptions to be set for different "repoview" objects backed by a same unfiltered repo, which seems more desirable.

That's a very good idea. I have split up the series and send the first part which adds the functionality.

quark accepted this revision.Nov 28 2017, 8:08 PM
quark added inline comments.
mercurial/repoview.py
237–239

Maybe rename exceptions to revs so people can know its type from the signature.

pulkit updated this revision to Diff 3956.Nov 28 2017, 9:06 PM
pulkit updated this revision to Diff 4051.Dec 1 2017, 11:40 AM
pulkit updated this revision to Diff 4092.Dec 4 2017, 9:16 AM
lothiraldan added inline comments.
mercurial/repoview.py
189

As _visibilityexceptions is defined as a class attribute, mutating it is likely to mutate it for all instances of repoview, I'm not sure if it's the intended behavior.

yuja requested changes to this revision.Dec 6 2017, 8:53 AM
yuja added a subscriber: yuja.

As _visibilityexceptions is defined as a class attribute, mutating it is likely to mutate it for all instances of repoview,

Indeed.

mercurial/localrepo.py
575

raise ProgrammingError?

577

Perhaps this can be a private method implemented in repoview.

This revision now requires changes to proceed.Dec 6 2017, 8:53 AM
pulkit added a comment.Dec 7 2017, 3:11 PM
In D1285#27283, @yuja wrote:

As _visibilityexceptions is defined as a class attribute, mutating it is likely to mutate it for all instances of repoview,

Indeed.

I have spend a lot of time on this and I am unable to make this work without making it class attribute because to calculate filterrevs, unfilteredrepo is passed and I lose the reference to the object on which I initially stored the visibilityexceptions. If I call repo.filtered(filtername), I am getting a new object with empty visibilityexception set.

@yuja what do you think about this approach https://phab.mercurial-scm.org/D1577#26875?

yuja added a comment.Dec 8 2017, 11:12 AM

I have spend a lot of time on this and I am unable to make this work without making it class attribute because to calculate filterrevs, unfilteredrepo is passed and I lose the reference to the object on which I initially stored the visibilityexceptions. If I call repo.filtered(filtername), I am getting a new object with empty visibilityexception set.

How long should the visibilityexception be preserved?

The safest one is to bind it to a temporary filteredrepo object, so that new .filtered()
object won't see it.

We could attach it to unfi, but in which case, we have to be careful to not leak
the current visibilityexception to the subsequent sessions. A repo object may
live longer than a single command/request session in hgweb or command server.

In D1285#27808, @yuja wrote:

I have spend a lot of time on this and I am unable to make this work without making it class attribute because to calculate filterrevs, unfilteredrepo is passed and I lose the reference to the object on which I initially stored the visibilityexceptions. If I call repo.filtered(filtername), I am getting a new object with empty visibilityexception set.

How long should the visibilityexception be preserved?

The visibility exceptions must be preserved until we calculate filteredrevs for that filter. After adding the expections, we clear filteredrevcache to make sure we calculate that again.

The safest one is to bind it to a temporary filteredrepo object, so that new .filtered()
object won't see it.
We could attach it to unfi, but in which case, we have to be careful to not leak
the current visibilityexception to the subsequent sessions. A repo object may
live longer than a single command/request session in hgweb or command server.

Since we want them while calculating filteredrevs where we pass a unfiltered repo, we need a way to get visibilityexceptions for a filtername from unfi. Maybe we can clear out the exceptions after using them.

quark added inline comments.Dec 8 2017, 7:52 PM
mercurial/localrepo.py
575

I guess hg log -r HASH --hidden would go through this code path so pass makes sense.

yuja added a comment.Dec 8 2017, 8:24 PM

How long should the visibilityexception be preserved?

The visibility exceptions must be preserved until we calculate filteredrevs for that filter. After adding the expections, we clear filteredrevcache to make sure we calculate that again.

Sounds like it is actually a temporary variable. Instead, how about making
filtertable or filterfunc dynamic?

repo.filtertable['visible:blahblah'] = partial(computehidden, exceptrevs)
repo.filtered('visible:blahblah')

We could attach it to unfi, but in which case, we have to be careful to not leak
the current visibilityexception to the subsequent sessions. A repo object may
live longer than a single command/request session in hgweb or command server.

Since we want them while calculating filteredrevs where we pass a unfiltered repo, we need a way to get visibilityexceptions for a filtername from unfi. Maybe we can clear out the exceptions after using them.

I guess the hard part is when we can say the exceptions are "used."

yuja added inline comments.Dec 8 2017, 8:28 PM
mercurial/localrepo.py
575

Maybe. My concern was getvisibilityexceptions() "seemed" not
working correctly in that case. I know unfiltered repo has no
exception, but I would expect get will return add-ed.

pulkit abandoned this revision.Jan 10 2018, 3:52 AM