hgext3rd/undo.py
35–39	The old code is fine. Usually we avoid moving code unnecessarily.
51	Might worth commenting the parameter type in `safelog`'s docstring if it has to be a list.
64–65	This could be simplified to one line later.
77	This makes `repo.currenttransaction()` work but is not logically right. The transaction object constructed here is only for writing a couple of revlogs we care about and nothing else. Other users (currently none, but it's better to be a good citizen) using `repo.transaction()` will accidentally use our transaction object which does not work with `hg recover` when bad things happen. I think we can remove this line, and do a `s/repo, /repo, tr, /` substitution for `log`, `_log*` and `writelog`. Then pass the `tr` object to them.
80	Worth a docstring saying when it returns True and when False.
204–205	This didn't get mentioned in commit message. Might update commit message or make it a separate change.
231	since you did a small cleanup here, might also add a space before `#`.
tests/test-undo.t
151	Is there a reason to use `head -1`? Usually we avoid that kind of filtering. If only certain fields needs to be tested, use `-T` to specify the template, like: hg log -r 'draft()' -T '{rev}:{node} {desc}\n' Otherwise the old code is fine, or use `>/dev/null` to discard the output.

hgext3rd/undo.py
70–73	Can you please explain how's lighttransaction different? It doesn't run hooks? Is it the only difference?

hgext3rd/undo.py
70–73	It avoids running anything that repo.transaction() runs except for transaction.transaction(). This is mostly hooks, but also includes some checks on locking, abandoned transactions and tag tracking. Calling lighttransaction, we should know that the repo is locked. Since lighttransaction is only used for recording information and doesn't actually perform any changes on the repo, ignoring tags and abandoned transactions should be fine.

hgext3rd/undo.py
64–65	Does this mean we take the lock for every command anyone ever runs? That will cause read operations to hang if something else is doing a pull/commit/etc. I don't think we can take the lock for every command.
92	Since this doesn't check for existing repo transactions, we could end up with two transactions running at once. This seems pretty dangerous. Could we assert that there is no current transaction, and block any repo transaction from attempting to open?

hgext3rd/undo.py
64–65	Good point. Replaced as discussed with undologlock, so we only hang if someone is currently logging.
92	This should not be dangerous as undolog doesn't edit the repo

hgext3rd/undo.py
68–70	Seems outdated? Without `timeout` we will be able to get the lock eventually.
72	Usually `desc` is a single word without space, like `undo`.

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	68		Jul 11 2017, 5:56 PM	★	★
Diff 2	104		Jul 13 2017, 12:41 AM	★	★
Diff 3	209		Jul 17 2017, 3:09 PM	★	★
Diff 4	218		Jul 17 2017, 4:10 PM	★	★
Diff 5	241	rFBHGX63d071a85d9d89c5979e75fe0aa12246f48db1bc	Jul 17 2017, 4:18 PM	★	★

Status	Author	Revision
Closed	felixmerk	D111 undo: file corruption handling
Closed	felixmerk	D108 undo: meaningful obs markers for undos
Closed	felixmerk	D55 undo: add --keep to maintian working copy changes
Closed	felixmerk	D53 undo: adds -f flag and gap edge case handling
Closed	felixmerk	D52 undo: implements relative undo, hg redo and tests
Closed	felixmerk	D51 undo: bare bones undo without tests
Closed	felixmerk	D50 undo: improved performance and prep for hg undo

Diff 68

hgext3rd/undo.py

	# undo.py: records data in revlog for future undo functionality			# undo.py: records data in revlog for future undo functionality
	#			#
	# Copyright 2017 Facebook, Inc.			# Copyright 2017 Facebook, Inc.
	#			#
	# This software may be used and distributed according to the terms of the			# This software may be used and distributed according to the terms of the
	# 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
	import os			import weakref

	from mercurial.i18n import _			from mercurial.i18n import _

	from mercurial import (			from mercurial import (
	dispatch,			dispatch,
	error,			error,
	extensions,			extensions,
	registrar,			registrar,
	revlog,			revlog,
	revset,			revset,
	revsetlang,			revsetlang,
	smartset,			smartset,
				transaction,
	util,			util,
	)			)

	from mercurial.node import (			from mercurial.node import (
	bin,			bin,
	hex,			hex,
	nullid,			nullid,
	)			)

	cmdtable = {}			cmdtable = {}
	command = registrar.command(cmdtable)			command = registrar.command(cmdtable)

				# Setup

				def extsetup(ui):
				extensions.wrapfunction(dispatch, 'runcommand', _runcommandwrapper)

				quarkUnsubmitted Not Done The old code is fine. Usually we avoid moving code unnecessarily. quark: The old code is fine. Usually we avoid moving code unnecessarily.
	# Wrappers			# Wrappers

	def _runcommandwrapper(orig, lui, repo, cmd, fullargs, *args):			def _runcommandwrapper(orig, lui, repo, cmd, fullargs, *args):
	# This wrapper executes whenever a command is run.			# This wrapper executes whenever a command is run.
	# Some commands (eg hg sl) don't actually modify anything			# Some commands (eg hg sl) don't actually modify anything
	# ie can't be undone, but the command doesn't know this.			# ie can't be undone, but the command doesn't know this.
	command = fullargs			command = fullargs

	# Check wether undolog is consistent			# Check wether undolog is consistent
	# ie check wether the undo ext was			# ie check wether the undo ext was
	# off before this command			# off before this command
	safelog(repo, "")			safelog(repo, [""])
				quarkUnsubmitted Not Done Might worth commenting the parameter type in `safelog`'s docstring if it has to be a list. quark: Might worth commenting the parameter type in `safelog`'s docstring if it has to be a list.

	result = orig(lui, repo, cmd, fullargs, *args)			result = orig(lui, repo, cmd, fullargs, *args)

	# record changes to repo			# record changes to repo
	safelog(repo, command)			safelog(repo, command)
	return result			return result

	# Write: Log control			# Write: Log control

	def safelog(repo, command):			def safelog(repo, command):
				changes = False
	if repo is not None:# some hg commands don't require repo			if repo is not None:# some hg commands don't require repo
	with repo.lock():			with repo.lock():
	with repo.transaction("undolog"):			with lighttransaction(repo):
				quarkUnsubmitted Not Done This could be simplified to one line later. quark: This could be simplified to one line later.
				durhamUnsubmitted Not Done Does this mean we take the lock for every command anyone ever runs? That will cause read operations to hang if something else is doing a pull/commit/etc. I don't think we can take the lock for every command. durham: Does this mean we take the lock for every command anyone ever runs? That will cause read…
				felixmerkAuthorUnsubmitted Not Done Good point. Replaced as discussed with undologlock, so we only hang if someone is currently logging. felixmerk: Good point. Replaced as discussed with undologlock, so we only hang if someone is currently…
	log(repo.filtered('visible'), command)			changes = log(repo.filtered('visible'), command)
				return changes

				def lighttransaction(repo):
				# full fledged transactions have two serious issues:
				quarkUnsubmitted Not Done Seems outdated? Without `timeout` we will be able to get the lock eventually. quark: Seems outdated? Without `timeout` we will be able to get the lock eventually.
				# 1. they may cause infite loops through hooks
				# that run commands
				quarkUnsubmitted Not Done Usually `desc` is a single word without space, like `undo`. quark: Usually `desc` is a single word without space, like `undo`.
				# 2. they are really expensive performance wise
				stashUnsubmitted Not Done Can you please explain how's lighttransaction different? It doesn't run hooks? Is it the only difference? stash: Can you please explain how's lighttransaction different? It doesn't run hooks? Is it the only…
				felixmerkAuthorUnsubmitted Not Done It avoids running anything that repo.transaction() runs except for transaction.transaction(). This is mostly hooks, but also includes some checks on locking, abandoned transactions and tag tracking. Calling lighttransaction, we should know that the repo is locked. Since lighttransaction is only used for recording information and doesn't actually perform any changes on the repo, ignoring tags and abandoned transactions should be fine. felixmerk: It avoids running anything that repo.transaction() runs except for transaction.transaction().
				vfsmap = {'plain': repo.vfs}
				tr = transaction.transaction(repo.ui.warn, repo.svfs, vfsmap,
				"_undojournal", "_undolog")
				repo._transref = weakref.ref(tr)
				quarkUnsubmitted Not Done This makes `repo.currenttransaction()` work but is not logically right. The transaction object constructed here is only for writing a couple of revlogs we care about and nothing else. Other users (currently none, but it's better to be a good citizen) using `repo.transaction()` will accidentally use our transaction object which does not work with `hg recover` when bad things happen. I think we can remove this line, and do a `s/repo, /repo, tr, /` substitution for `log`, `_log` and `writelog`. Then pass the `tr` object to them. quark:* This makes `repo.currenttransaction()` work but is not logically right. The transaction object…
				return tr

	def log(repo, command):			def log(repo, command):
				quarkUnsubmitted Not Done Worth a docstring saying when it returns True and when False. quark: Worth a docstring saying when it returns True and when False.
	newnodes = {			newnodes = {
	'bookmarks': _logbookmarks(repo),			'bookmarks': _logbookmarks(repo),
	'draftheads': _logdraftheads(repo),			'draftheads': _logdraftheads(repo),
	'workingparent': _logworkingparent(repo),			'workingparent': _logworkingparent(repo),
	}			}
	try:			try:
	exsistingnodes = _readindex(repo, 0)			exsistingnodes = _readindex(repo, 0)
	except IndexError:			except IndexError:
	exsistingnodes = {}			exsistingnodes = {}
	if all(newnodes.get(x) == exsistingnodes.get(x) for x in newnodes.keys()):			if all(newnodes.get(x) == exsistingnodes.get(x) for x in newnodes.keys()):
	return			return False
	else:			else:
				durhamUnsubmitted Not Done Since this doesn't check for existing repo transactions, we could end up with two transactions running at once. This seems pretty dangerous. Could we assert that there is no current transaction, and block any repo transaction from attempting to open? durham: Since this doesn't check for existing repo transactions, we could end up with two transactions…
				felixmerkAuthorUnsubmitted Not Done This should not be dangerous as undolog doesn't edit the repo felixmerk: This should not be dangerous as undolog doesn't edit the repo
	newnodes.update({			newnodes.update({
	'date': _logdate(repo),			'date': _logdate(repo),
	'command': _logcommand(repo, command),			'command': _logcommand(repo, command),
	})			})
	_logindex(repo, newnodes)			_logindex(repo, newnodes)
				return True

	# Write: Logs			# Write: Logs

	def writelog(repo, name, revstring):			def writelog(repo, name, revstring):
	assert repo.currenttransaction() is not None			tr = repo.currenttransaction()
	# The transaction code doesn't work with vfs			assert tr is not None
				# the transaction code doesn't work with vfs
	# specifically, repo.recover() assumes svfs?			# specifically, repo.recover() assumes svfs?
	repo.svfs.makedirs('undolog')			repo.svfs.makedirs('undolog')
	path = os.path.join('undolog', name)			rlog = _getrevlog(repo, name)
	rlog = revlog.revlog(repo.svfs, path)
	tr = repo.currenttransaction()
	node = rlog.addrevision(revstring, tr, 1, nullid, nullid)			node = rlog.addrevision(revstring, tr, 1, nullid, nullid)
	return hex(node)			return hex(node)

	def _logdate(repo):			def _logdate(repo):
	revstring = " ".join(str(x) for x in util.makedate())			revstring = " ".join(str(x) for x in util.makedate())
	return writelog(repo, "date.i", revstring)			return writelog(repo, "date.i", revstring)

	def _logdraftheads(repo):			def _logdraftheads(repo):
	def _logindex(repo, nodes):			def _logindex(repo, nodes):
	revstring = "\n".join(sorted('%s %s' % (k, v) for k, v in nodes.items()))			revstring = "\n".join(sorted('%s %s' % (k, v) for k, v in nodes.items()))
	return writelog(repo, "index.i", revstring)			return writelog(repo, "index.i", revstring)

	# Read			# Read

	def _readindex(repo, reverseindex, prefetchedrevlog=None):			def _readindex(repo, reverseindex, prefetchedrevlog=None):
	if prefetchedrevlog is None:			if prefetchedrevlog is None:
	path = os.path.join('undolog', 'index.i')			rlog = _getrevlog(repo, 'index.i')
	rlog = revlog.revlog(repo.svfs, path)
	else:			else:
	rlog = prefetchedrevlog			rlog = prefetchedrevlog
	index = len(rlog) - reverseindex - 1			index = _invertindex(rlog, reverseindex)
	if index < 0 or index > len(rlog) - 1:			if index < 0 or index > len(rlog) - 1:
	raise IndexError			raise IndexError
	chunk = rlog.revision(index)			chunk = rlog.revision(index)
	indexdict = {}			indexdict = {}
	for row in chunk.split("\n"):			for row in chunk.split("\n"):
	kvpair = row.split(' ', 1)			kvpair = row.split(' ', 1)
	if kvpair[0]:			if kvpair[0]:
	indexdict[kvpair[0]] = kvpair[1]			indexdict[kvpair[0]] = kvpair[1]
	return indexdict			return indexdict

	def _readnode(repo, filename, hexnode):			def _readnode(repo, filename, hexnode):
	path = os.path.join('undolog', filename)			rlog = _getrevlog(repo, filename)
	rlog = revlog.revlog(repo.svfs, path)
	return rlog.revision(bin(hexnode))			return rlog.revision(bin(hexnode))

	# Visualize			# Visualize

	"""debug commands and instrumentation for the undo extension			"""debug commands and instrumentation for the undo extension

	Adds the `debugundohistory` and `debugundosmartlog` commands to visualize			Adds the `debugundohistory` and `debugundosmartlog` commands to visualize
	operational history and to give a preview of how undo will behave.			operational history and to give a preview of how undo will behave.
	"""			"""

	@command('debugundohistory', [			@command('debugundohistory', [
	('n', 'index', 0, _("details about specific operation")),			('n', 'index', 0, _("details about specific operation")),
	('l', 'list', False, _("list recent undo-able operation"))			('l', 'list', False, _("list recent undo-able operation"))
	])			])
	def debugundohistory(ui, repo, args, *kwargs):			def debugundohistory(ui, repo, args, *opts):
	""" Print operational history			""" Print operational history
	0 is the most recent operation			0 is the most recent operation
	"""			"""
	if repo is not None:			if repo is not None:
	if kwargs.get('list'):			if opts.get('list'):
	if args and args[0].isdigit():			if args and args[0].isdigit():
	offset = int(args[0])			offset = int(args[0])
	else:			else:
	offset = 0			offset = 0
	_debugundolist(ui, repo, offset)			_debugundolist(ui, repo, offset)
	else:			else:
	reverseindex = kwargs.get('index')			reverseindex = opts.get('index')
	if 0 == reverseindex and args and args[0].isdigit():			if 0 == reverseindex and args and args[0].isdigit():
	reverseindex = int(args[0])			reverseindex = int(args[0])
	_debugundoindex(ui, repo, reverseindex)			_debugundoindex(ui, repo, reverseindex)

	def _debugundolist(ui, repo, offset):			def _debugundolist(ui, repo, offset):
	offset = abs(offset)			offset = abs(offset)

	template = "{sub('\0', ' ', undo)}\n"			template = "{sub('\0', ' ', undo)}\n"
	fm = ui.formatter('debugundohistory', {'template': template})			fm = ui.formatter('debugundohistory', {'template': template})
	path = os.path.join('undolog', 'index.i')			prefetchedrevlog = _getrevlog(repo, 'index.i')

	prefetchedrevlog = revlog.revlog(repo.svfs, path)
	recentrange = min(5, len(prefetchedrevlog) - offset)			recentrange = min(5, len(prefetchedrevlog) - offset)
	if 0 == recentrange:			if 0 == recentrange:
	fm.startitem()			fm.startitem()
	fm.write('undo', '%s', "None")			fm.write('undo', '%s', "None")
	for i in range(recentrange):			for i in range(recentrange):
	nodedict = _readindex(repo, i + offset, prefetchedrevlog)			nodedict = _readindex(repo, i + offset, prefetchedrevlog)
	commandstr = _readnode(repo, 'command.i', nodedict['command'])			commandstr = _readnode(repo, 'command.i', nodedict['command'])
				if "" == commandstr:
				commandstr = " -- gap in log -- "
				quarkUnsubmitted Not Done This didn't get mentioned in commit message. Might update commit message or make it a separate change. quark: This didn't get mentioned in commit message. Might update commit message or make it a separate…
	fm.startitem()			fm.startitem()
	fm.write('undo', '%s', str(i + offset) + ": " + commandstr)			fm.write('undo', '%s', str(i + offset) + ": " + commandstr)
	fm.end()			fm.end()

	def _debugundoindex(ui, repo, reverseindex):			def _debugundoindex(ui, repo, reverseindex):
	try:			try:
	nodedict = _readindex(repo, reverseindex)			nodedict = _readindex(repo, reverseindex)
	except IndexError:			except IndexError:
	if "date.i" == filename:			if "date.i" == filename:
	splitdate = rawcontent.split(" ")			splitdate = rawcontent.split(" ")
	datetuple = (float(splitdate[0]), int(splitdate[1]))			datetuple = (float(splitdate[0]), int(splitdate[1]))
	content = util.datestr(datetuple)			content = util.datestr(datetuple)
	elif "draftheads.i" == filename:			elif "draftheads.i" == filename:
	try:			try:
	oldnodes = _readindex(repo, reverseindex + 1)			oldnodes = _readindex(repo, reverseindex + 1)
	oldheads = _readnode(repo, filename, oldnodes[filename[:-2]])			oldheads = _readnode(repo, filename, oldnodes[filename[:-2]])
	except IndexError:# Index is oldest log			except IndexError:# index is oldest log
				quarkUnsubmitted Not Done since you did a small cleanup here, might also add a space before `#`. quark: since you did a small cleanup here, might also add a space before `#`.
	content = rawcontent			content = rawcontent
	else:			else:
	content = "ADDED:\n\t" + "\n\t".join(sorted(			content = "ADDED:\n\t" + "\n\t".join(sorted(
	set(rawcontent.split("\n"))			set(rawcontent.split("\n"))
	- set(oldheads.split("\n"))			- set(oldheads.split("\n"))
	))			))
	content += "\nREMOVED:\n\t" + "\n\t".join(sorted(			content += "\nREMOVED:\n\t" + "\n\t".join(sorted(
	set(oldheads.split("\n"))			set(oldheads.split("\n"))
	Note: this revset may include hidden commits			Note: this revset may include hidden commits
	"""			"""
	args = revset.getargsdict(x, 'olddraftrevset', 'reverseindex')			args = revset.getargsdict(x, 'olddraftrevset', 'reverseindex')
	reverseindex = revsetlang.getinteger(args.get('reverseindex'),			reverseindex = revsetlang.getinteger(args.get('reverseindex'),
	_('index must be a positive integer'), 1)			_('index must be a positive integer'), 1)
	revs = _getolddrafts(repo, reverseindex)			revs = _getolddrafts(repo, reverseindex)
	return smartset.baseset(revs)			return smartset.baseset(revs)

	# Setup			# Tools

	def extsetup(ui):			def _invertindex(rlog, indexorreverseindex):
	extensions.wrapfunction(dispatch, 'runcommand', _runcommandwrapper)			return len(rlog) - 1 - indexorreverseindex

				def _getrevlog(repo, filename):
				path = 'undolog/' + filename
				return revlog.revlog(repo.svfs, path)

tests/test-undo.t

	$ touch cmiss && hg add cmiss && hg ci -mcmiss			$ touch cmiss && hg add cmiss && hg ci -mcmiss
	$ cat >>$HGRCPATH <<EOF			$ cat >>$HGRCPATH <<EOF
	> [extensions]			> [extensions]
	> undo = $TESTDIR/../hgext3rd/undo.py			> undo = $TESTDIR/../hgext3rd/undo.py
	> EOF			> EOF
	$ touch a5 && hg add a5 && hg ci -ma5			$ touch a5 && hg add a5 && hg ci -ma5
	$ hg debugundohistory -l			$ hg debugundohistory -l
	0: ci -ma5			0: ci -ma5
	1:			1: -- gap in log --
	2: commit -m words			2: commit -m words
	3: update master			3: update master
	4: commit --amend			4: commit --amend
	$ hg debugundohistory 1			$ hg debugundohistory 1
	command:			command:
	unkown command(s) run, gap in log			unkown command(s) run, gap in log
	bookmarks:			bookmarks:
	feature1 49cdb4091aca3c09f402ff001cd20cf086873683			feature1 49cdb4091aca3c09f402ff001cd20cf086873683
	1dafc0b436123cab96f82a8e9e8d1d42c0301aaa			1dafc0b436123cab96f82a8e9e8d1d42c0301aaa

	Index out of bound error			Index out of bound error
	$ hg debugundohistory -n 50			$ hg debugundohistory -n 50
	abort: index out of bounds			abort: index out of bounds
	[255]			[255]

	Revset tests			Revset tests
	$ hg log -G -r 'draft()' --hidden >> c1			$ hg log -G -r 'draft()' --hidden \| head -1
				quarkUnsubmitted Not Done Is there a reason to use `head -1`? Usually we avoid that kind of filtering. If only certain fields needs to be tested, use `-T` to specify the template, like: hg log -r 'draft()' -T '{rev}:{node} {desc}\n' Otherwise the old code is fine, or use `>/dev/null` to discard the output. quark: Is there a reason to use `head -1`? Usually we avoid that kind of filtering. If only certain…
				@ changeset: 8:aa430c8afedf
	$ hg debugundohistory -n 0			$ hg debugundohistory -n 0
	command:			command:
	ci -ma5			ci -ma5
	bookmarks:			bookmarks:
	feature1 49cdb4091aca3c09f402ff001cd20cf086873683			feature1 49cdb4091aca3c09f402ff001cd20cf086873683
	feature2 296fda51a303650465d07a1cd054075cbe6d3cbd			feature2 296fda51a303650465d07a1cd054075cbe6d3cbd
	master aa430c8afedf9b2ec3f0655d39eef6b6b0a2ddb6			master aa430c8afedf9b2ec3f0655d39eef6b6b0a2ddb6
	date:			date:

			Path	Packages
M			hgext3rd/undo.py (71 lines)
M			tests/test-undo.t (5 lines)

This is an archive of the discontinued Mercurial Phabricator instance.

undo: improved performance and prep for hg undo
ClosedPublic

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 68

hgext3rd/undo.py

tests/test-undo.t

This is an archive of the discontinued Mercurial Phabricator instance.

undo: improved performance and prep for hg undoClosedPublic

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 68

hgext3rd/undo.py

tests/test-undo.t

undo: improved performance and prep for hg undo
ClosedPublic

Revision Contents
Changeset List