This is an archive of the discontinued Mercurial Phabricator instance.

Differential D165

uncommit: keep parent phase for full uncommit
ClosedPublic

Authored by mbthomas on Jul 21 2017, 7:43 AM.

Download Raw Diff

Details

Reviewers

ryanmce

Group Reviewers

Restricted Project

Commits

rFBHGXf6510b66b72e: uncommit: keep parent phase for full uncommit

Summary

When fully uncommitting a commit on top of a public commit, uncommit
erroneously marks the new current commit with the same phase as the old
commit. It should only do that in the case of a partial commit,
where the new current commit is a new commit.

Test Plan

Add tests to test-uncommit.t.

Diff Detail

Repository

rFBHGX Facebook Mercurial Extensions

Branch

mbt-uncommit (bookmark) on default (branch)

Lint

Lint OK

Unit

Unit Tests OK

Event Timeline

mbthomas created this revision.Jul 21 2017, 7:43 AM

I think you can make this change more surgical by just checking the hash at the caller.

hgext3rd/uncommit.py
166	Rather than passing back an additional parameter , can't we check if the `newid` is `old.parent`'s hash? Then this change will be less invasive.

Compare IDs to determine if the new current commit is new

mbthomas marked an inline comment as done.Jul 21 2017, 8:20 AM

awesome, thanks for the fix!

This revision is now accepted and ready to land.Jul 21 2017, 8:25 AM

Closed by commit rFBHGXf6510b66b72e: uncommit: keep parent phase for full uncommit (authored by mbthomas). · Explain WhyJul 21 2017, 11:10 AM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			hgext3rd/uncommit.py (20 lines)
M			tests/test-uncommit.t (25 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	357		Jul 21 2017, 7:43 AM	★	★
Diff 2	359	Compare IDs to determine if the new current commit is new	Jul 21 2017, 8:19 AM	★	★
Diff 3	372	rFBHGXf6510b66b72e8770528b299f74b4c7de282396ab	Jul 21 2017, 8:27 AM	★	★

Commit	Local	Parents	Author	Summary	Date
6cab90eabbc0	3388	53d2c31bbdaa	Mark Thomas	uncommit: keep parent phase for full uncommit	Jul 21 2017, 6:25 AM

Diff 357

hgext3rd/uncommit.py

	oldbookmarks = repo.nodebookmarks(oldid)			oldbookmarks = repo.nodebookmarks(oldid)
	if oldbookmarks:			if oldbookmarks:
	changes = []			changes = []
	for b in oldbookmarks:			for b in oldbookmarks:
	changes.append((b, newid))			changes.append((b, newid))
	repo._bookmarks.applychanges(repo, tr, changes)			repo._bookmarks.applychanges(repo, tr, changes)

	def _commitfiltered(repo, ctx, match):			def _commitfiltered(repo, ctx, match):
	"""Recommit ctx with changed files not in match. Return the new			"""Recommit ctx with changed files not in match. Returns tuple of
	node identifier, or None if nothing changed.			(newid, isnew), where newid is the new node identifier, or None if
				nothing changed, and isnew is True if the new node is a new commit.
	"""			"""
	base = ctx.p1()			base = ctx.p1()
	# ctx			# ctx
	initialfiles = set(ctx.files())			initialfiles = set(ctx.files())
	exclude = set(f for f in initialfiles if match(f))			exclude = set(f for f in initialfiles if match(f))

	# No files matched commit, so nothing excluded			# No files matched commit, so nothing excluded
	if not exclude:			if not exclude:
	return None			return None, False

	files = (initialfiles - exclude)			files = (initialfiles - exclude)
	if not files:			if not files:
	return ctx.parents()[0].node()			return ctx.parents()[0].node(), False

	# Filter copies			# Filter copies
	copied = copies.pathcopies(base, ctx)			copied = copies.pathcopies(base, ctx)
	copied = dict((dst, src) for dst, src in copied.iteritems()			copied = dict((dst, src) for dst, src in copied.iteritems()
	if dst in files)			if dst in files)
	def filectxfn(repo, memctx, path, contentctx=ctx, redirect=()):			def filectxfn(repo, memctx, path, contentctx=ctx, redirect=()):
	if path not in contentctx:			if path not in contentctx:
	return None			return None
	fctx = contentctx[path]			fctx = contentctx[path]
	mctx = context.memfilectx(repo, fctx.path(), fctx.data(),			mctx = context.memfilectx(repo, fctx.path(), fctx.data(),
	fctx.islink(),			fctx.islink(),
	fctx.isexec(),			fctx.isexec(),
	copied=copied.get(path))			copied=copied.get(path))
	return mctx			return mctx

	new = context.memctx(repo,			new = context.memctx(repo,
	parents=[base.node(), node.nullid],			parents=[base.node(), node.nullid],
	text=ctx.description(),			text=ctx.description(),
	files=files,			files=files,
	filectxfn=filectxfn,			filectxfn=filectxfn,
	user=ctx.user(),			user=ctx.user(),
	date=ctx.date(),			date=ctx.date(),
	extra=ctx.extra())			extra=ctx.extra())
	newid = repo.commitctx(new)			newid = repo.commitctx(new)
	return newid			return newid, True

	def _uncommitdirstate(repo, oldctx, match):			def _uncommitdirstate(repo, oldctx, match):
	"""Fix the dirstate after switching the working directory from			"""Fix the dirstate after switching the working directory from
	oldctx to a copy of oldctx not containing changed files matched by			oldctx to a copy of oldctx not containing changed files matched by
	match.			match.
	"""			"""
	ctx = repo['.']			ctx = repo['.']
	ds = repo.dirstate			ds = repo.dirstate
	oldphase = old.phase()			oldphase = old.phase()
	if oldphase == phases.public:			if oldphase == phases.public:
	raise error.Abort(_("cannot rewrite immutable changeset"))			raise error.Abort(_("cannot rewrite immutable changeset"))
	if len(old.parents()) > 1:			if len(old.parents()) > 1:
	raise error.Abort(_("cannot uncommit merge changeset"))			raise error.Abort(_("cannot uncommit merge changeset"))

	with repo.transaction('uncommit') as tr:			with repo.transaction('uncommit') as tr:
	match = scmutil.match(old, pats, opts)			match = scmutil.match(old, pats, opts)
	newid = _commitfiltered(repo, old, match)			newid, isnew = _commitfiltered(repo, old, match)
	if newid is None:			if newid is None:
	raise error.Abort(_('nothing to uncommit'))			raise error.Abort(_('nothing to uncommit'))

	# Move local changes on filtered changeset			# Move local changes on filtered changeset
				if isnew:
				ryanmceUnsubmitted Done Rather than passing back an additional parameter , can't we check if the `newid` is `old.parent`'s hash? Then this change will be less invasive. ryanmce: Rather than passing back an additional parameter , can't we check if the `newid` is `old.
	obsolete.createmarkers(repo, [(old, (repo[newid],))])			obsolete.createmarkers(repo, [(old, (repo[newid],))])
	phases.retractboundary(repo, tr, oldphase, [newid])			phases.retractboundary(repo, tr, oldphase, [newid])
				else:
				obsolete.createmarkers(repo, [(old, ())])

	with repo.dirstate.parentchange():			with repo.dirstate.parentchange():
	repo.dirstate.setparents(newid, node.nullid)			repo.dirstate.setparents(newid, node.nullid)
	_uncommitdirstate(repo, old, match)			_uncommitdirstate(repo, old, match)

	_updatebookmarks(repo, old.node(), newid, tr)			_updatebookmarks(repo, old.node(), newid, tr)

tests/test-uncommit.t

	\| \| o 3:6db330d65db434145c0b59d291853e9a84719b24 added file-abcd			\| \| o 3:6db330d65db434145c0b59d291853e9a84719b24 added file-abcd
	\| \| \|			\| \| \|
	\| \| x 2:abf2df566fc193b3ac34d946e63c1583e4d4732b added file-abc			\| \| x 2:abf2df566fc193b3ac34d946e63c1583e4d4732b added file-abc
	\| \|/			\| \|/
	\| x 1:69a232e754b08d568c4899475faf2eb44b857802 added file-ab			\| x 1:69a232e754b08d568c4899475faf2eb44b857802 added file-ab
	\|/			\|/
	o 0:3004d2d9b50883c1538fc754a3aeb55f1b4084f6 added file-a			o 0:3004d2d9b50883c1538fc754a3aeb55f1b4084f6 added file-a

				Uncommit with draft parent

				$ hg uncommit
				$ hg phase -r .
				10: draft
				$ hg commit -m 'update ab again'

				Uncommit with public parent

				$ hg phase -p "::.^"
				$ hg uncommit
				$ hg phase -r .
				10: public

				Partial uncommit with public parent

				$ echo xyz > xyz
				$ hg add xyz
				$ hg commit -m "update ab and add xyz"
				$ hg uncommit xyz
				$ hg status
				A xyz
				$ hg phase -r .
				14: draft
				$ hg phase -r ".^"
				10: public