This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1285

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 """

pulkit added a comment.Nov 20 2017, 2:48 PM
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.
pulkit added a comment.Nov 22 2017, 8:26 PM
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 a subscriber: lothiraldan.Dec 6 2017, 4:49 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.

pulkit added a comment.Dec 8 2017, 12:07 PM
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

Revision Contents
Changeset List

PathPackages
Mmercurial/localrepo.py (8 lines)
Mmercurial/repoview.py (11 lines)

Diff 4092

mercurial/localrepo.py

# Key to signature value. # Key to signature value.
self._sparsesignaturecache = {} self._sparsesignaturecache = {}
# Signature to cached matcher instance. # Signature to cached matcher instance.
self._sparsematchercache = {} self._sparsematchercache = {}
def _getvfsward(self, origfunc): def _getvfsward(self, origfunc):
"""build a ward for self.vfs""" """build a ward for self.vfs"""
rref = weakref.ref(self) rref = weakref.ref(self)
def checkvfs(path, mode=None): def checkvfs(path, mode=None):
quarkUnsubmitted
Not Done

nit: do not need a space around """

quark: nit: do not need a space around `"""`
ret = origfunc(path, mode=mode) ret = origfunc(path, mode=mode)
repo = rref() repo = rref()
if (repo is None if (repo is None
or not util.safehasattr(repo, '_wlockref') or not util.safehasattr(repo, '_wlockref')
or not util.safehasattr(repo, '_lockref')): or not util.safehasattr(repo, '_lockref')):
return return
if mode in (None, 'r', 'rb'): if mode in (None, 'r', 'rb'):
return return
repo.ui.develwarn('write with no lock: "%s"' % path, repo.ui.develwarn('write with no lock: "%s"' % path,
stacklevel=3) stacklevel=3)
return ret return ret
return checksvfs return checksvfs
def close(self): def close(self):
self._writecaches() self._writecaches()
def addvisibilityexceptions(self, exceptions):
# should be called on a filtered repository
pass
yujaUnsubmitted
Not Done

raise ProgrammingError?

yuja: raise ProgrammingError?
quarkUnsubmitted
Not Done

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

quark: I guess `hg log -r HASH --hidden` would go through this code path so `pass` makes sense.
yujaUnsubmitted
Not Done

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.

yuja: Maybe. My concern was `getvisibilityexceptions()` "seemed" not working correctly in that case.
def getvisibilityexceptions(self):
yujaUnsubmitted
Not Done

Perhaps this can be a private method implemented in repoview.

yuja: Perhaps this can be a private method implemented in repoview.
# should be called on a filtered repository
return set()
def _loadextensions(self): def _loadextensions(self):
extensions.loadall(self.ui) extensions.loadall(self.ui)
def _writecaches(self): def _writecaches(self):
if self._revbranchcache: if self._revbranchcache:
self._revbranchcache.write() self._revbranchcache.write()
def _restrictcapabilities(self, caps): def _restrictcapabilities(self, caps):

mercurial/repoview.py

leads to the property below. leads to the property below.
repoview.method() --> repo.__class__.method(repoview) repoview.method() --> repo.__class__.method(repoview)
The inheritance has to be done dynamically because `repo` can be of any The inheritance has to be done dynamically because `repo` can be of any
subclasses of `localrepo`. Eg: `bundlerepo` or `statichttprepo`. subclasses of `localrepo`. Eg: `bundlerepo` or `statichttprepo`.
""" """
# hidden revs which should be visible
_visibilityexceptions = set()
lothiraldanUnsubmitted
Not Done

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.

lothiraldan: As `_visibilityexceptions` is defined as a class attribute, mutating it is likely to mutate it…
def __init__(self, repo, filtername): def __init__(self, repo, filtername):
object.__setattr__(self, r'_unfilteredrepo', repo) object.__setattr__(self, r'_unfilteredrepo', repo)
object.__setattr__(self, r'filtername', filtername) object.__setattr__(self, r'filtername', filtername)
object.__setattr__(self, r'_clcachekey', None) object.__setattr__(self, r'_clcachekey', None)
object.__setattr__(self, r'_clcache', None) object.__setattr__(self, r'_clcache', None)
# not a propertycache on purpose we shall implement a proper cache later # not a propertycache on purpose we shall implement a proper cache later
@property @property
return self._unfilteredrepo return self._unfilteredrepo
def filtered(self, name): def filtered(self, name):
"""Return a filtered version of a repository""" """Return a filtered version of a repository"""
if name == self.filtername: if name == self.filtername:
return self return self
return self.unfiltered().filtered(name) return self.unfiltered().filtered(name)
def addvisibilityexceptions(self, revs):
"""adds hidden revs which should be visible to set of exceptions"""
self._visibilityexceptions.update(revs)
quarkUnsubmitted
Not Done

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

quark: Maybe rename `exceptions` to `revs` so people can know its type from the signature.
def getvisibilityexceptions(self):
"""returns the set of hidden revs which should be visible"""
return self._visibilityexceptions
# everything access are forwarded to the proxied repo # everything access are forwarded to the proxied repo
def __getattr__(self, attr): def __getattr__(self, attr):
return getattr(self._unfilteredrepo, attr) return getattr(self._unfilteredrepo, attr)
def __setattr__(self, attr, value): def __setattr__(self, attr, value):
return setattr(self._unfilteredrepo, attr, value) return setattr(self._unfilteredrepo, attr, value)
def __delattr__(self, attr): def __delattr__(self, attr):
return delattr(self._unfilteredrepo, attr) return delattr(self._unfilteredrepo, attr)