Patchwork D2969: context: move reuse of context object to repo.__getitem__ (API)

login
register
mail settings
Submitter phabricator
Date March 30, 2018, 6:10 a.m.
Message ID <differential-rev-PHID-DREV-t7jvbtyu6qqtojzl4pp4-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/29950/
State Superseded
Headers show

Comments

phabricator - March 30, 2018, 6:10 a.m.
martinvonz created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  As an example of how weird the basectx.__new__ is: whenever you create
  a workingctx, basectx.__new__ gets called first. Since our __new__ has
  a "changeid" argument as second parameter, when create the
  workingctx(repo, text="blah"), the text gets bound to
  "changeid". Since a string isn't a basectx, our __new__ ends up not
  doing anything funny, but that's still very confusing code.
  
  Another case is metadataonlyctx.__new__(), which I think exists in
  order to prevent metadataonlyctx.__init__'s third argument
  (originalctx) from being interpreted as a changeid in
  basectx.__new__(), thereby getting reused.
  
  Let's move this to repo.__getitem__ instead, where it will be pretty
  obvious what the code does.
  
  After this patch, changectx(ctx) will be an error (it will fail when
  trying to see if it's a 20-byte string).

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/context.py
  mercurial/localrepo.py

CHANGE DETAILS




To: martinvonz, #hg-reviewers
Cc: mercurial-devel
phabricator - March 30, 2018, 12:26 p.m.
yuja added subscribers: smf, yuja.
yuja added a comment.


  +1, but I don't remember why we made that hack.
  
  @smf Any thoughts?

REPOSITORY
  rHG Mercurial

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

To: martinvonz, #hg-reviewers
Cc: yuja, smf, mercurial-devel
phabricator - March 30, 2018, 5:27 p.m.
indygreg accepted this revision.
indygreg added a comment.
This revision is now accepted and ready to land.


  That's 2 of us who think this is a good idea. I'm going to queue it.
  
  If @smf knows of a reason why this is bad, we can always drop it or revert it.

REPOSITORY
  rHG Mercurial

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

To: martinvonz, #hg-reviewers, indygreg
Cc: indygreg, yuja, smf, mercurial-devel

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -782,6 +782,8 @@ 
     def __getitem__(self, changeid):
         if changeid is None:
             return context.workingctx(self)
+        if isinstance(changeid, context.basectx):
+            return changeid
         if isinstance(changeid, slice):
             # wdirrev isn't contiguous so the slice shouldn't include it
             return [context.changectx(self, i)
diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -62,11 +62,6 @@ 
                 be committed,
     memctx: a context that represents changes in-memory and can also
             be committed."""
-    def __new__(cls, repo, changeid='', *args, **kwargs):
-        if isinstance(changeid, basectx):
-            return changeid
-
-        return super(basectx, cls).__new__(cls)
 
     def __bytes__(self):
         return short(self.node())
@@ -412,11 +407,6 @@ 
     def __init__(self, repo, changeid='.'):
         """changeid is a revision number, node, or tag"""
 
-        # since basectx.__new__ already took care of copying the object, we
-        # don't need to do anything in __init__, so we just exit here
-        if isinstance(changeid, basectx):
-            return
-
         if changeid == '':
             changeid = '.'
         self._repo = repo
@@ -2486,9 +2476,6 @@ 
     dateutil.parsedate() and defaults to current date, extra is a dictionary of
     metadata or is left empty.
     """
-    def __new__(cls, repo, originalctx, *args, **kwargs):
-        return super(metadataonlyctx, cls).__new__(cls, repo)
-
     def __init__(self, repo, originalctx, parents=None, text=None, user=None,
                  date=None, extra=None, editor=False):
         if text is None: