Patchwork D1285: localrepo: add a new attribute _visibilityexceptions and related API

login
register
mail settings
Submitter phabricator
Date Nov. 2, 2017, 6:04 p.m.
Message ID <differential-rev-PHID-DREV-j3rjfvsetkqtvhk6dxye-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/25350/
State Superseded
Headers show

Comments

phabricator - Nov. 2, 2017, 6:04 p.m.
pulkit created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION 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 localrepository class which will contains
  the hidden revs which should be exception.
  
  This patch also adds API to add revs to the attribute set and get them.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1285

AFFECTED FILES
  mercurial/localrepo.py

CHANGE DETAILS




To: pulkit, #hg-reviewers
Cc: mercurial-devel
phabricator - Nov. 9, 2017, 8:55 p.m.
quark added a comment.


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

INLINE COMMENTS

> localrepo.py:516
> +    def addvisibilityexceptions(self, exceptions):
> +        """ adds hidden revs which should be visible to set of exceptions """
> +        self._visibilityexceptions.update(exceptions)

nit: do not need a space around `"""`

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1285

To: pulkit, #hg-reviewers
Cc: quark, mercurial-devel
phabricator - Nov. 20, 2017, 7:48 p.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1285

To: pulkit, #hg-reviewers
Cc: quark, mercurial-devel
phabricator - Nov. 20, 2017, 9:20 p.m.
quark added a comment.


  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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1285

To: pulkit, #hg-reviewers
Cc: quark, mercurial-devel
phabricator - Nov. 23, 2017, 1:26 a.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1285

To: pulkit, #hg-reviewers
Cc: quark, mercurial-devel
phabricator - Nov. 29, 2017, 1:08 a.m.
quark accepted this revision.
quark added inline comments.

INLINE COMMENTS

> repoview.py:236-238
> +    def addvisibilityexceptions(self, exceptions):
> +        """adds hidden revs which should be visible to set of exceptions"""
> +        self._visibilityexceptions.update(exceptions)

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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1285

To: pulkit, #hg-reviewers, quark
Cc: quark, mercurial-devel
phabricator - Dec. 6, 2017, 9:49 a.m.
lothiraldan added inline comments.

INLINE COMMENTS

> repoview.py:189
> +    # hidden revs which should be visible
> +    _visibilityexceptions = set()
> +

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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1285

To: pulkit, #hg-reviewers, quark
Cc: lothiraldan, quark, mercurial-devel
phabricator - Dec. 6, 2017, 1:53 p.m.
yuja requested changes to this revision.
yuja added a comment.
This revision now requires changes to proceed.


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

INLINE COMMENTS

> localrepo.py:575
> +        # should be called on a filtered repository
> +        pass
> +

raise ProgrammingError?

> localrepo.py:577
> +
> +    def getvisibilityexceptions(self):
> +        # should be called on a filtered repository

Perhaps this can be a private method implemented in repoview.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1285

To: pulkit, #hg-reviewers, quark, yuja
Cc: yuja, lothiraldan, quark, mercurial-devel
phabricator - Dec. 7, 2017, 8:11 p.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/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?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1285

To: pulkit, #hg-reviewers, quark, yuja
Cc: yuja, lothiraldan, quark, mercurial-devel
phabricator - Dec. 8, 2017, 4:12 p.m.
yuja added a comment.


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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1285

To: pulkit, #hg-reviewers, quark, yuja
Cc: yuja, lothiraldan, quark, mercurial-devel
phabricator - Dec. 8, 2017, 5:07 p.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1285

To: pulkit, #hg-reviewers, quark, yuja
Cc: yuja, lothiraldan, quark, mercurial-devel
phabricator - Dec. 9, 2017, 12:52 a.m.
quark added inline comments.

INLINE COMMENTS

> yuja wrote in localrepo.py:575
> raise ProgrammingError?

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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1285

To: pulkit, #hg-reviewers, quark, yuja
Cc: yuja, lothiraldan, quark, mercurial-devel
phabricator - Dec. 9, 2017, 1:24 a.m.
yuja added a comment.


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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1285

To: pulkit, #hg-reviewers, quark, yuja
Cc: yuja, lothiraldan, quark, mercurial-devel
phabricator - Dec. 9, 2017, 1:28 a.m.
yuja added inline comments.

INLINE COMMENTS

> quark wrote in localrepo.py:575
> I guess `hg log -r HASH --hidden` would go through this code path so `pass` makes sense.

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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1285

To: pulkit, #hg-reviewers, quark, yuja
Cc: yuja, lothiraldan, quark, mercurial-devel

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -509,6 +509,16 @@ 
         self._sparsesignaturecache = {}
         # Signature to cached matcher instance.
         self._sparsematchercache = {}
+        # hidden hashed which should be visible
+        self._visibilityexceptions = set()
+
+    def addvisibilityexceptions(self, exceptions):
+        """ adds hidden revs which should be visible to set of exceptions """
+        self._visibilityexceptions.update(exceptions)
+
+    def getvisibilityexceptions(self):
+        """ returns the set of hidden revs which should be visible """
+        return self._visibilityexceptions
 
     def _getvfsward(self, origfunc):
         """build a ward for self.vfs"""