This is an archive of the discontinued Mercurial Phabricator instance.

Differential D2692

merge: return an attrs class from update() and applyupdates()
ClosedPublic

Authored by indygreg on Mar 5 2018, 1:17 AM.

Details

Reviewers
martinvonz
durin42
Group Reviewers
hg-reviewers
Commits
rHG71543b942eea: merge: return an attrs class from update() and applyupdates()
Summary

Previously, we returned a tuple containing counts. The result of an
update is kind of complex and the use of tuples with nameless fields
made the code a bit harder to read and constrained future expansion
of the return value.

Let's invent an attrs-defined class for representing the result of
an update operation.

We provide getitem and len implementations for backwards
compatibility as a container type to minimize code churn.

In (at least) Python 2, the % operator seems to insist on using
tuples. So we had to update a consumer using the % operator.

.. api::

merge.update() and merge.applyupdates() now return a class
with named attributes instead of a tuple. Switch consumers
to access elements by name instead of by offset.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

indygreg created this revision.Mar 5 2018, 1:17 AM
indygreg updated this revision to Diff 6969.Mar 12 2018, 6:28 PM
martinvonz added a subscriber: martinvonz.Mar 21 2018, 8:15 PM
martinvonz added inline comments.
mercurial/merge.py
1403–1404

mpm made me use a tuple with slots for scmutil.status. Any idea when that's better? I think it was something about performance, but we don't care about that here.

martinvonz accepted this revision.Mar 21 2018, 8:16 PM
This revision is now accepted and ready to land.Mar 21 2018, 8:16 PM
indygreg added a comment.Mar 21 2018, 8:20 PM

__slots__ allocates enough space for the exact set of attributes specified rather than backing object instances by __dict__. If you create thousands of small objects that all have the same fields, __slots__ can yield some performance wins.

durin42 accepted this revision.Mar 24 2018, 3:07 PM
Closed by commit rHG71543b942eea: merge: return an attrs class from update() and applyupdates() (authored by indygreg). · Explain WhyMar 24 2018, 5:21 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/hg.py (4 lines)
Mmercurial/merge.py (35 lines)

Diff 6652

mercurial/hg.py

if srcpeer is not None: if srcpeer is not None:
srcpeer.close() srcpeer.close()
return srcpeer, destpeer return srcpeer, destpeer
def _showstats(repo, stats, quietempty=False): def _showstats(repo, stats, quietempty=False):
if quietempty and not any(stats): if quietempty and not any(stats):
return return
repo.ui.status(_("%d files updated, %d files merged, " repo.ui.status(_("%d files updated, %d files merged, "
"%d files removed, %d files unresolved\n") % stats) "%d files removed, %d files unresolved\n") % (
stats.updatedcount, stats.mergedcount,
stats.removedcount, stats.unresolvedcount))
def updaterepo(repo, node, overwrite, updatecheck=None): def updaterepo(repo, node, overwrite, updatecheck=None):
"""Update the working directory to node. """Update the working directory to node.
When overwrite is set, changes are clobbered, merged else When overwrite is set, changes are clobbered, merged else
returns stats (see pydoc mercurial.merge.applyupdates)""" returns stats (see pydoc mercurial.merge.applyupdates)"""
return mergemod.update(repo, node, False, overwrite, return mergemod.update(repo, node, False, overwrite,

mercurial/merge.py

addednodeid, addednodeid,
bin, bin,
hex, hex,
modifiednodeid, modifiednodeid,
nullhex, nullhex,
nullid, nullid,
nullrev, nullrev,
) )
from .thirdparty import (
attr,
)
from . import ( from . import (
copies, copies,
error, error,
filemerge, filemerge,
match as matchmod, match as matchmod,
obsutil, obsutil,
pycompat, pycompat,
scmutil, scmutil,
# Skipping 'a', 'am', 'f', 'r', 'dm', 'e', 'k', 'p' and 'pr', because they # Skipping 'a', 'am', 'f', 'r', 'dm', 'e', 'k', 'p' and 'pr', because they
# don't touch the context to be merged in. 'cd' is skipped, because # don't touch the context to be merged in. 'cd' is skipped, because
# changed/deleted never resolves to something from the remote side. # changed/deleted never resolves to something from the remote side.
oplist = [actions[a] for a in 'g dc dg m'.split()] oplist = [actions[a] for a in 'g dc dg m'.split()]
prefetch = scmutil.fileprefetchhooks prefetch = scmutil.fileprefetchhooks
prefetch(repo, ctx, [f for sublist in oplist for f, args, msg in sublist]) prefetch(repo, ctx, [f for sublist in oplist for f, args, msg in sublist])
@attr.s(frozen=True)
class updateresult(object):
martinvonzUnsubmitted
Not Done

mpm made me use a tuple with slots for scmutil.status. Any idea when that's better? I think it was something about performance, but we don't care about that here.

martinvonz: mpm made me use a tuple with __slots__ for scmutil.status. Any idea when that's better? I think…
updatedcount = attr.ib()
mergedcount = attr.ib()
removedcount = attr.ib()
unresolvedcount = attr.ib()
# TODO remove container emulation once consumers switch to new API.
def __getitem__(self, x):
if x == 0:
return self.updatedcount
elif x == 1:
return self.mergedcount
elif x == 2:
return self.removedcount
elif x == 3:
return self.unresolvedcount
else:
raise IndexError('can only access items 0-3')
def __len__(self):
return 4
def applyupdates(repo, actions, wctx, mctx, overwrite, labels=None): def applyupdates(repo, actions, wctx, mctx, overwrite, labels=None):
"""apply the merge action list to the working directory """apply the merge action list to the working directory
wctx is the working copy context wctx is the working copy context
mctx is the context to be merged into the working copy mctx is the context to be merged into the working copy
Return a tuple of counts (updated, merged, removed, unresolved) that Return a tuple of counts (updated, merged, removed, unresolved) that
describes how many files were affected by the update. describes how many files were affected by the update.
"support mergedriver") "support mergedriver")
ms.commit() ms.commit()
proceed = driverpreprocess(repo, ms, wctx, labels=labels) proceed = driverpreprocess(repo, ms, wctx, labels=labels)
# the driver might leave some files unresolved # the driver might leave some files unresolved
unresolvedf = set(ms.unresolved()) unresolvedf = set(ms.unresolved())
if not proceed: if not proceed:
# XXX setting unresolved to at least 1 is a hack to make sure we # XXX setting unresolved to at least 1 is a hack to make sure we
# error out # error out
return updated, merged, removed, max(len(unresolvedf), 1) return updateresult(updated, merged, removed,
max(len(unresolvedf), 1))
newactions = [] newactions = []
for f, args, msg in mergeactions: for f, args, msg in mergeactions:
if f in unresolvedf: if f in unresolvedf:
newactions.append((f, args, msg)) newactions.append((f, args, msg))
mergeactions = newactions mergeactions = newactions
try: try:
# premerge # premerge
# #
# We don't need to do the same operation for 'dc' and 'cd' because # We don't need to do the same operation for 'dc' and 'cd' because
# those lists aren't consulted again. # those lists aren't consulted again.
mfiles.difference_update(a[0] for a in acts) mfiles.difference_update(a[0] for a in acts)
actions['m'] = [a for a in actions['m'] if a[0] in mfiles] actions['m'] = [a for a in actions['m'] if a[0] in mfiles]
progress(_updating, None, total=numupdates, unit=_files) progress(_updating, None, total=numupdates, unit=_files)
return updateresult(updated, merged, removed, unresolved)
return updated, merged, removed, unresolved
def recordupdates(repo, actions, branchmerge): def recordupdates(repo, actions, branchmerge):
"record merge actions to the dirstate" "record merge actions to the dirstate"
# remove (must come first) # remove (must come first)
for f, args, msg in actions.get('r', []): for f, args, msg in actions.get('r', []):
if branchmerge: if branchmerge:
repo.dirstate.remove(f) repo.dirstate.remove(f)
else: else:
for s in sorted(wc.substate): for s in sorted(wc.substate):
wc.sub(s).bailifchanged() wc.sub(s).bailifchanged()
elif not overwrite: elif not overwrite:
if p1 == p2: # no-op update if p1 == p2: # no-op update
# call the hooks and exit early # call the hooks and exit early
repo.hook('preupdate', throw=True, parent1=xp2, parent2='') repo.hook('preupdate', throw=True, parent1=xp2, parent2='')
repo.hook('update', parent1=xp2, parent2='', error=0) repo.hook('update', parent1=xp2, parent2='', error=0)
return 0, 0, 0, 0 return updateresult(0, 0, 0, 0)
if (updatecheck == 'linear' and if (updatecheck == 'linear' and
pas not in ([p1], [p2])): # nonlinear pas not in ([p1], [p2])): # nonlinear
dirty = wc.dirty(missing=True) dirty = wc.dirty(missing=True)
if dirty: if dirty:
# Branching is a bit strange to ensure we do the minimal # Branching is a bit strange to ensure we do the minimal
# amount of call to obsutil.foreground. # amount of call to obsutil.foreground.
foreground = obsutil.foreground(repo, [p1.node()]) foreground = obsutil.foreground(repo, [p1.node()])