( )⚙ D5994 cleanup: prefer nested context managers to \-continuations

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D5994

cleanup: prefer nested context managers to \-continuations
ClosedPublic

Authored by durin42 on Feb 20 2019, 7:33 PM.

Details

Reviewers
martinvonz
Group Reviewers
hg-reviewers
Commits
rHG1eb2fc21da12: cleanup: prefer nested context managers to \-continuations
Summary

I'd prefer Python accept a tuple of context managers, but alas it
isn't meant to be. This will have to suffice.

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

durin42 created this revision.Feb 20 2019, 7:33 PM
martinvonz added a comment.Feb 20 2019, 7:42 PM

I'd prefer Python accept a tuple of context managers

I think I'd also prefer if our context managers didn't acquire the resource in __init__. That would let us write it with less indentation as:

wlock = wlock or util.nullcontextmanager()
lock = lock or util.nullcontextmanager()
trmanager = pushop.trmanager or util.nullcontextmanager()
with wlock, lock, trmanager:

(In this case we probably could do that anyway since we don't seem to worry about the risk of exceptions from the time we take the lock until we enter the context manager.)

I don't care much either way about the patch itself. I'll wait a little to see if someone else cares more, but otherwise I'll take.

durin42 added a comment.Feb 20 2019, 7:46 PM
In D5994#87506, @martinvonz wrote:

I'd prefer Python accept a tuple of context managers

I think I'd also prefer if our context managers didn't acquire the resource in __init__. That would let us write it with less indentation as:

wlock = wlock or util.nullcontextmanager()
lock = lock or util.nullcontextmanager()
trmanager = pushop.trmanager or util.nullcontextmanager()
with wlock, lock, trmanager:

(In this case we probably could do that anyway since we don't seem to worry about the risk of exceptions from the time we take the lock until we enter the context manager.)
I don't care much either way about the patch itself. I'll wait a little to see if someone else cares more, but otherwise I'll take.

That sounds like a good cleanup anyway - probably taking the resource is a vestige of when we were on around Python 2.3 and mpm had this discipline of using del to free things like locks. It worked, but isn't idiomatic today (I'm honestly not sure if it was then - but mpm is where I learned it.)

Closed by commit rHG1eb2fc21da12: cleanup: prefer nested context managers to \-continuations (authored by durin42). · Explain WhyFeb 21 2019, 4:28 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 14179

hgext/largefiles/overrides.py

if (lfutil.shortname in src and if (lfutil.shortname in src and
dest.startswith(repo.wjoin(lfutil.shortname))): dest.startswith(repo.wjoin(lfutil.shortname))):
destlfile = dest.replace(lfutil.shortname, '') destlfile = dest.replace(lfutil.shortname, '')
if not opts['force'] and os.path.exists(destlfile): if not opts['force'] and os.path.exists(destlfile):
raise IOError('', raise IOError('',
_('destination largefile already exists')) _('destination largefile already exists'))
copiedfiles.append((src, dest)) copiedfiles.append((src, dest))
orig(src, dest, *args, **kwargs) orig(src, dest, *args, **kwargs)
with extensions.wrappedfunction(util, 'copyfile', overridecopyfile), \ with extensions.wrappedfunction(util, 'copyfile', overridecopyfile):
extensions.wrappedfunction(scmutil, 'match', overridematch): with extensions.wrappedfunction(scmutil, 'match', overridematch):
result += orig(ui, repo, listpats, opts, rename) result += orig(ui, repo, listpats, opts, rename)
lfdirstate = lfutil.openlfdirstate(ui, repo) lfdirstate = lfutil.openlfdirstate(ui, repo)
for (src, dest) in copiedfiles: for (src, dest) in copiedfiles:
if (lfutil.shortname in src and if (lfutil.shortname in src and
dest.startswith(repo.wjoin(lfutil.shortname))): dest.startswith(repo.wjoin(lfutil.shortname))):
srclfile = src.replace(repo.wjoin(lfutil.standin('')), '') srclfile = src.replace(repo.wjoin(lfutil.standin('')), '')
destlfile = dest.replace(repo.wjoin(lfutil.standin('')), '') destlfile = dest.replace(repo.wjoin(lfutil.standin('')), '')
destlfiledir = repo.wvfs.dirname(repo.wjoin(destlfile)) or '.' destlfiledir = repo.wvfs.dirname(repo.wjoin(destlfile)) or '.'

hgext/narrow/narrowcommands.py

with ui.uninterruptible(): with ui.uninterruptible():
common = commoninc[0] common = commoninc[0]
if ellipsesremote: if ellipsesremote:
ds = repo.dirstate ds = repo.dirstate
p1, p2 = ds.p1(), ds.p2() p1, p2 = ds.p1(), ds.p2()
with ds.parentchange(): with ds.parentchange():
ds.setparents(node.nullid, node.nullid) ds.setparents(node.nullid, node.nullid)
with wrappedextraprepare,\ with wrappedextraprepare:
repo.ui.configoverride(overrides, 'widen'): with repo.ui.configoverride(overrides, 'widen'):
exchange.pull(repo, remote, heads=common) exchange.pull(repo, remote, heads=common)
with ds.parentchange(): with ds.parentchange():
ds.setparents(p1, p2) ds.setparents(p1, p2)
else: else:
with remote.commandexecutor() as e: with remote.commandexecutor() as e:
bundle = e.callcommand('narrow_widen', { bundle = e.callcommand('narrow_widen', {
'oldincludes': oldincludes, 'oldincludes': oldincludes,
'oldexcludes': oldexcludes, 'oldexcludes': oldexcludes,
'newincludes': newincludes, 'newincludes': newincludes,
'newexcludes': newexcludes, 'newexcludes': newexcludes,
'cgversion': '03', 'cgversion': '03',
'commonheads': common, 'commonheads': common,
'known': [], 'known': [],
'ellipses': False, 'ellipses': False,
}).result() }).result()
with repo.transaction('widening') as tr,\ with repo.transaction('widening') as tr:
repo.ui.configoverride(overrides, 'widen'): with repo.ui.configoverride(overrides, 'widen'):
tgetter = lambda: tr tgetter = lambda: tr
bundle2.processbundle(repo, bundle, bundle2.processbundle(repo, bundle,
transactiongetter=tgetter) transactiongetter=tgetter)
with repo.transaction('widening'): with repo.transaction('widening'):
repo.setnewnarrowpats() repo.setnewnarrowpats()
narrowspec.updateworkingcopy(repo) narrowspec.updateworkingcopy(repo)
narrowspec.copytoworkingcopy(repo) narrowspec.copytoworkingcopy(repo)
# TODO(rdamazio): Make new matcher format and update description # TODO(rdamazio): Make new matcher format and update description
@command('tracked', @command('tracked',

mercurial/exchange.py

except error.LockUnavailable as err: except error.LockUnavailable as err:
# source repo cannot be locked. # source repo cannot be locked.
# We do not abort the push, but just disable the local phase # We do not abort the push, but just disable the local phase
# synchronisation. # synchronisation.
msg = ('cannot lock source repository: %s\n' msg = ('cannot lock source repository: %s\n'
% stringutil.forcebytestr(err)) % stringutil.forcebytestr(err))
pushop.ui.debug(msg) pushop.ui.debug(msg)
with wlock or util.nullcontextmanager(), \ with wlock or util.nullcontextmanager():
lock or util.nullcontextmanager(), \ with lock or util.nullcontextmanager():
pushop.trmanager or util.nullcontextmanager(): with pushop.trmanager or util.nullcontextmanager():
pushop.repo.checkpush(pushop) pushop.repo.checkpush(pushop)
_checkpublish(pushop) _checkpublish(pushop)
_pushdiscovery(pushop) _pushdiscovery(pushop)
if not _forcebundle1(pushop): if not _forcebundle1(pushop):
_pushbundle2(pushop) _pushbundle2(pushop)
_pushchangeset(pushop) _pushchangeset(pushop)
_pushsyncphase(pushop) _pushsyncphase(pushop)
_pushobsolete(pushop) _pushobsolete(pushop)
_pushbookmark(pushop) _pushbookmark(pushop)
if repo.ui.configbool('experimental', 'remotenames'): if repo.ui.configbool('experimental', 'remotenames'):
logexchange.pullremotenames(repo, remote) logexchange.pullremotenames(repo, remote)
return pushop return pushop
# list of steps to perform discovery before push # list of steps to perform discovery before push
pushdiscoveryorder = [] pushdiscoveryorder = []