This is an archive of the discontinued Mercurial Phabricator instance.

Differential D3748

import: use context manager for lock, dirstateguard, transaction
ClosedPublic

Authored by martinvonz on Jun 14 2018, 7:32 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG2ceea1554d1e: import: use context manager for lock, dirstateguard, transaction
Summary

A tiny side-effect is that the transaction is now closed after saving
the commit message.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

martinvonz created this revision.Jun 14 2018, 7:32 PM
yuja added a subscriber: yuja.Jun 15 2018, 8:39 AM

+ if not opts.get('no_commit'):
+ lock = repo.lock()
+ tr = repo.transaction('import')
+ dsguard = util.nullcontextmanager()
+ else:
+ lock = util.nullcontextmanager()
+ tr = util.nullcontextmanager()
+ dsguard = dirstateguard.dirstateguard(repo, 'import')
+ with lock, tr, dsguard:

Doesn't it leave a stale lock if repo.transaction() raises exception?

martinvonz updated this revision to Diff 9095.Jun 15 2018, 1:04 PM
martinvonz added a comment.Jun 15 2018, 1:04 PM
In D3748#58738, @yuja wrote:

+ if not opts.get('no_commit'):
+ lock = repo.lock()
+ tr = repo.transaction('import')
+ dsguard = util.nullcontextmanager()
+ else:
+ lock = util.nullcontextmanager()
+ tr = util.nullcontextmanager()
+ dsguard = dirstateguard.dirstateguard(repo, 'import')
+ with lock, tr, dsguard:

Doesn't it leave a stale lock if repo.transaction() raises exception?

Good catch. I have often wished that our transactions and locks respected the context manager protocol and did the locking in enter. How do you feel about this workaround (see updated code)?

yuja added a comment.Jun 15 2018, 10:00 PM

+ if not opts.get('no_commit'):
+ lock = repo.lock()
+ try:

tr = repo.transaction('import')
  • else:
  • dsguard = dirstateguard.dirstateguard(repo, 'import')

+ except:
+ lock.release()

Needs to re-raise.

+ dsguard = util.nullcontextmanager()
+ else:
+ lock = util.nullcontextmanager()
+ tr = util.nullcontextmanager()
+ dsguard = dirstateguard.dirstateguard(repo, 'import')
+ with lock, tr, dsguard:

Another option is to wrap transaction and dirstateguard by lambda or partial,
and call them with "with".

with lock(), tr(), dsguard():
martinvonz updated this revision to Diff 9127.Jun 17 2018, 9:33 AM
martinvonz added a comment.Jun 17 2018, 9:33 AM
In D3748#58833, @yuja wrote:

+ if not opts.get('no_commit'):
+ lock = repo.lock()
+ try:

tr = repo.transaction('import')
  • else:
  • dsguard = dirstateguard.dirstateguard(repo, 'import')

+ except:
+ lock.release()

Needs to re-raise.

+ dsguard = util.nullcontextmanager()
+ else:
+ lock = util.nullcontextmanager()
+ tr = util.nullcontextmanager()
+ dsguard = dirstateguard.dirstateguard(repo, 'import')
+ with lock, tr, dsguard:

Another option is to wrap transaction and dirstateguard by lambda or partial,
and call them with "with".

with lock(), tr(), dsguard():

Good idea. Done.

Closed by commit rHG2ceea1554d1e: import: use context manager for lock, dirstateguard, transaction (authored by martinvonz). · Explain WhyJun 18 2018, 8:32 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/commands.py (40 lines)

Diff 9095

mercurial/commands.py

error, error,
exchange, exchange,
extensions, extensions,
formatter, formatter,
graphmod, graphmod,
hbisect, hbisect,
help, help,
hg, hg,
lock as lockmod,
logcmdutil, logcmdutil,
merge as mergemod, merge as mergemod,
obsolete, obsolete,
obsutil, obsutil,
patch, patch,
phases, phases,
pycompat, pycompat,
rcutil, rcutil,
util, util,
wireprotoserver, wireprotoserver,
) )
from .utils import ( from .utils import (
dateutil, dateutil,
stringutil, stringutil,
) )
release = lockmod.release
table = {} table = {}
table.update(debugcommandsmod.command._table) table.update(debugcommandsmod.command._table)
command = registrar.command(table) command = registrar.command(table)
INTENT_READONLY = registrar.INTENT_READONLY INTENT_READONLY = registrar.INTENT_READONLY
# common command options # common command options
raise error.Abort(_('cannot use --similarity with --bypass')) raise error.Abort(_('cannot use --similarity with --bypass'))
if exact: if exact:
if opts.get('edit'): if opts.get('edit'):
raise error.Abort(_('cannot use --exact with --edit')) raise error.Abort(_('cannot use --exact with --edit'))
if opts.get('prefix'): if opts.get('prefix'):
raise error.Abort(_('cannot use --exact with --prefix')) raise error.Abort(_('cannot use --exact with --prefix'))
base = opts["base"] base = opts["base"]
dsguard = lock = tr = None
msgs = [] msgs = []
ret = 0 ret = 0
with repo.wlock(): with repo.wlock():
try:
if update: if update:
cmdutil.checkunfinished(repo) cmdutil.checkunfinished(repo)
if (exact or not opts.get('force')): if (exact or not opts.get('force')):
cmdutil.bailifchanged(repo) cmdutil.bailifchanged(repo)
if not opts.get('no_commit'): if not opts.get('no_commit'):
lock = repo.lock() lock = repo.lock()
try:
tr = repo.transaction('import') tr = repo.transaction('import')
except:
lock.release()
dsguard = util.nullcontextmanager()
else: else:
lock = util.nullcontextmanager()
tr = util.nullcontextmanager()
dsguard = dirstateguard.dirstateguard(repo, 'import') dsguard = dirstateguard.dirstateguard(repo, 'import')
with lock, tr, dsguard:
parents = repo[None].parents() parents = repo[None].parents()
for patchurl in patches: for patchurl in patches:
if patchurl == '-': if patchurl == '-':
ui.status(_('applying patch from stdin\n')) ui.status(_('applying patch from stdin\n'))
patchfile = ui.fin patchfile = ui.fin
patchurl = 'stdin' # for error message patchurl = 'stdin' # for error message
else: else:
patchurl = os.path.join(base, patchurl) patchurl = os.path.join(base, patchurl)
ui.write_err(_("(fix the .rej files and run " ui.write_err(_("(fix the .rej files and run "
"`hg commit --amend`)\n")) "`hg commit --amend`)\n"))
ret = 1 ret = 1
break break
if not haspatch: if not haspatch:
raise error.Abort(_('%s: no diffs found') % patchurl) raise error.Abort(_('%s: no diffs found') % patchurl)
if tr:
tr.close()
if msgs: if msgs:
repo.savecommitmessage('\n* * *\n'.join(msgs)) repo.savecommitmessage('\n* * *\n'.join(msgs))
if dsguard:
dsguard.close()
return ret return ret
finally:
if tr:
tr.release()
release(lock, dsguard)
@command('incoming|in', @command('incoming|in',
[('f', 'force', None, [('f', 'force', None,
_('run even if remote repository is unrelated')), _('run even if remote repository is unrelated')),
('n', 'newest-first', None, _('show newest record first')), ('n', 'newest-first', None, _('show newest record first')),
('', 'bundle', '', ('', 'bundle', '',
_('file to store the bundles into'), _('FILE')), _('file to store the bundles into'), _('FILE')),
('r', 'rev', [], _('a remote changeset intended to be added'), _('REV')), ('r', 'rev', [], _('a remote changeset intended to be added'), _('REV')),