This is an archive of the discontinued Mercurial Phabricator instance.

fold: use context manager for locks and transaction
ClosedPublic

Authored by pulkit on Oct 17 2017, 8:14 PM.

Download Raw Diff

Details

Reviewers

mjpieters
durham

Group Reviewers

Restricted Project

Commits

rFBHGX7698bb9a21d8: fold: use context manager for locks and transaction

Diff Detail

Repository

rFBHGX Facebook Mercurial Extensions

Lint

Lint Skipped

Unit

Unit Tests Skipped

Event Timeline

pulkit created this revision.Oct 17 2017, 8:14 PM

Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 17 2017, 8:14 PM

mjpieters requested changes to this revision.Oct 19 2017, 9:46 AM

mjpieters added a subscriber: mjpieters.

mjpieters added inline comments.

hgext3rd/fbamend/fold.py
110–111	This swaps the order of the transaction being entered and the foldcheck being done. I suspect this order matters. Creating a transaction is certainly quite involved, if the foldcheck fails we can avoid all that cost. Please add a separate `with repo.transaction('fold') as tr:` after the `_foldcheck(...)` line.

This revision now requires changes to proceed.Oct 19 2017, 9:46 AM

mjpieters added a child revision: D1040: fold: start using formatter to print output.Oct 19 2017, 9:48 AM

pulkit added inline comments.Oct 19 2017, 7:49 PM

hgext3rd/fbamend/fold.py
110–111

pulkit updated this revision to Diff 3138.Oct 27 2017, 6:04 PM

durham accepted this revision.Oct 31 2017, 1:02 PM

Closed by commit rFBHGX7698bb9a21d8: fold: use context manager for locks and transaction (authored by pulkit). · Explain WhyOct 31 2017, 4:23 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			hgext3rd/fbamend/fold.py (16 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	2959		Oct 17 2017, 8:14 PM	★	★
Diff 2	3138		Oct 27 2017, 6:04 PM	★	★
Diff 3	3183	rFBHGX7698bb9a21d815c5d925c34a3b3e38e61a05af3d	Oct 31 2017, 4:22 PM	★	★

Status	Author	Revision
Closed	pulkit	D1041 fold: add support to get json output for node changes
Closed	pulkit	D1040 fold: start using formatter to print output
Closed	pulkit	D1039 fold: use scmutil.cleanupnodes()
Closed	pulkit	D1038 tweakdefaults: add capability to skip some commands in cleanupnodes wrapper
Closed	pulkit	D1174 fold: use context manager for locks and transaction

Diff 3138

hgext3rd/fbamend/fold.py

	# GNU General Public License version 2 or any later version.			# GNU General Public License version 2 or any later version.

	from __future__ import absolute_import			from __future__ import absolute_import

	from mercurial import (			from mercurial import (
	commands,			commands,
	error,			error,
	hg,			hg,
	lock as lockmod,
	phases,			phases,
	registrar,			registrar,
	scmutil,			scmutil,
	)			)
	from mercurial.i18n import _			from mercurial.i18n import _

	from . import common			from . import common


	if not revs:			if not revs:
	raise error.Abort(_('specified revisions evaluate to an empty set'),			raise error.Abort(_('specified revisions evaluate to an empty set'),
	hint=_('use different revision arguments'))			hint=_('use different revision arguments'))
	elif len(revs) == 1:			elif len(revs) == 1:
	ui.write_err(_('single revision specified, nothing to fold\n'))			ui.write_err(_('single revision specified, nothing to fold\n'))
	return 1			return 1

	wlock = lock = None			with repo.wlock(), repo.lock():
	try:
	wlock = repo.wlock()
	lock = repo.lock()

	root, head = _foldcheck(repo, revs)			root, head = _foldcheck(repo, revs)
				mjpietersUnsubmitted Not Done This swaps the order of the transaction being entered and the foldcheck being done. I suspect this order matters. Creating a transaction is certainly quite involved, if the foldcheck fails we can avoid all that cost. Please add a separate `with repo.transaction('fold') as tr:` after the `_foldcheck(...)` line. mjpieters: This swaps the order of the transaction being entered and the foldcheck being done. I suspect…
				pulkitAuthorUnsubmitted Not Done pulkit: {meme, src=goodcatch}

	tr = repo.transaction('fold')			with repo.transaction('fold') as tr:
	try:
	commitopts = opts.copy()			commitopts = opts.copy()
	allctx = [repo[r] for r in revs]			allctx = [repo[r] for r in revs]
	targetphase = max(c.phase() for c in allctx)			targetphase = max(c.phase() for c in allctx)

	if commitopts.get('message') or commitopts.get('logfile'):			if commitopts.get('message') or commitopts.get('logfile'):
	commitopts['edit'] = False			commitopts['edit'] = False
	else:			else:
	msgs = ["HG: This is a fold of %d changesets." % len(allctx)]			msgs = ["HG: This is a fold of %d changesets." % len(allctx)]
	ui.status(_('%i changesets folded\n') % len(revs))			ui.status(_('%i changesets folded\n') % len(revs))
	if repo['.'].rev() in revs:			if repo['.'].rev() in revs:
	hg.update(repo, newid)			hg.update(repo, newid)

	if torebase:			if torebase:
	folded = repo.revs('allsuccessors(%ld)', revs).last()			folded = repo.revs('allsuccessors(%ld)', revs).last()
	common.restackonce(ui, repo, folded)			common.restackonce(ui, repo, folded)

	tr.close()
	finally:
	tr.release()
	finally:
	lockmod.release(lock, wlock)

	def _foldcheck(repo, revs):			def _foldcheck(repo, revs):
	roots = repo.revs('roots(%ld)', revs)			roots = repo.revs('roots(%ld)', revs)
	if len(roots) > 1:			if len(roots) > 1:
	raise error.Abort(_("cannot fold non-linear revisions "			raise error.Abort(_("cannot fold non-linear revisions "
	"(multiple roots given)"))			"(multiple roots given)"))
	root = repo[roots.first()]			root = repo[roots.first()]
	if root.phase() <= phases.public:			if root.phase() <= phases.public:
	raise error.Abort(_("cannot fold public revisions"))			raise error.Abort(_("cannot fold public revisions"))
	heads = repo.revs('heads(%ld)', revs)			heads = repo.revs('heads(%ld)', revs)
	if len(heads) > 1:			if len(heads) > 1:
	raise error.Abort(_("cannot fold non-linear revisions "			raise error.Abort(_("cannot fold non-linear revisions "
	"(multiple heads given)"))			"(multiple heads given)"))
	head = repo[heads.first()]			head = repo[heads.first()]
	return root, head			return root, head