This is an archive of the discontinued Mercurial Phabricator instance.

Differential D2969

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

Authored by martinvonz on Mar 30 2018, 2:09 AM.

Details

Reviewers
indygreg
Group Reviewers
hg-reviewers
Commits
rHGbb47dc2f71a0: context: move reuse of context object to repo.__getitem__ (API)
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).

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

martinvonz created this revision.Mar 30 2018, 2:09 AM
yuja added subscribers: smf, yuja.Mar 30 2018, 8:26 AM

+1, but I don't remember why we made that hack.

@smf Any thoughts?

indygreg accepted this revision.Mar 30 2018, 1:27 PM
indygreg added a subscriber: indygreg.

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.

This revision is now accepted and ready to land.Mar 30 2018, 1:27 PM
Closed by commit rHGbb47dc2f71a0: context: move reuse of context object to repo.__getitem__ (API) (authored by martinvonz). · Explain WhyMar 30 2018, 1:29 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/context.py (13 lines)
Mmercurial/localrepo.py (2 lines)

Diff 7392

mercurial/context.py

class basectx(object): class basectx(object):
"""A basectx object represents the common logic for its children: """A basectx object represents the common logic for its children:
changectx: read-only context that is already present in the repo, changectx: read-only context that is already present in the repo,
workingctx: a context that represents the working directory and can workingctx: a context that represents the working directory and can
be committed, be committed,
memctx: a context that represents changes in-memory and can also memctx: a context that represents changes in-memory and can also
be committed.""" be committed."""
def __new__(cls, repo, changeid='', *args, **kwargs):
if isinstance(changeid, basectx):
return changeid
return super(basectx, cls).__new__(cls)
def __bytes__(self): def __bytes__(self):
return short(self.node()) return short(self.node())
__str__ = encoding.strmethod(__bytes__) __str__ = encoding.strmethod(__bytes__)
def __repr__(self): def __repr__(self):
return r"<%s %s>" % (type(self).__name__, str(self)) return r"<%s %s>" % (type(self).__name__, str(self))
class changectx(basectx): class changectx(basectx):
"""A changecontext object makes access to data related to a particular """A changecontext object makes access to data related to a particular
changeset convenient. It represents a read-only context already present in changeset convenient. It represents a read-only context already present in
the repo.""" the repo."""
def __init__(self, repo, changeid='.'): def __init__(self, repo, changeid='.'):
"""changeid is a revision number, node, or tag""" """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 == '': if changeid == '':
changeid = '.' changeid = '.'
self._repo = repo self._repo = repo
try: try:
if isinstance(changeid, int): if isinstance(changeid, int):
self._node = repo.changelog.node(changeid) self._node = repo.changelog.node(changeid)
self._rev = changeid self._rev = changeid
'parents' is a sequence of two parent revisions identifiers (pass None for 'parents' is a sequence of two parent revisions identifiers (pass None for
every missing parent), 'text' is the commit. every missing parent), 'text' is the commit.
user receives the committer name and defaults to current repository user receives the committer name and defaults to current repository
username, date is the commit date in any format supported by username, date is the commit date in any format supported by
dateutil.parsedate() and defaults to current date, extra is a dictionary of dateutil.parsedate() and defaults to current date, extra is a dictionary of
metadata or is left empty. 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, def __init__(self, repo, originalctx, parents=None, text=None, user=None,
date=None, extra=None, editor=False): date=None, extra=None, editor=False):
if text is None: if text is None:
text = originalctx.description() text = originalctx.description()
super(metadataonlyctx, self).__init__(repo, text, user, date, extra) super(metadataonlyctx, self).__init__(repo, text, user, date, extra)
self._rev = None self._rev = None
self._node = None self._node = None
self._originalctx = originalctx self._originalctx = originalctx

mercurial/localrepo.py

from . import hg from . import hg
target = hg.sharedreposource(self) target = hg.sharedreposource(self)
narrowspec.save(target, newincludes, newexcludes) narrowspec.save(target, newincludes, newexcludes)
self.invalidate(clearfilecache=True) self.invalidate(clearfilecache=True)
def __getitem__(self, changeid): def __getitem__(self, changeid):
if changeid is None: if changeid is None:
return context.workingctx(self) return context.workingctx(self)
if isinstance(changeid, context.basectx):
return changeid
if isinstance(changeid, slice): if isinstance(changeid, slice):
# wdirrev isn't contiguous so the slice shouldn't include it # wdirrev isn't contiguous so the slice shouldn't include it
return [context.changectx(self, i) return [context.changectx(self, i)
for i in xrange(*changeid.indices(len(self))) for i in xrange(*changeid.indices(len(self)))
if i not in self.changelog.filteredrevs] if i not in self.changelog.filteredrevs]
try: try:
return context.changectx(self, changeid) return context.changectx(self, changeid)
except error.WdirUnsupported: except error.WdirUnsupported: