This is an archive of the discontinued Mercurial Phabricator instance.

pushrebase: check sources of renames when looking for conflicts
ClosedPublic

Authored by mbthomas on Oct 20 2017, 6:10 AM.

Download Raw Diff

Details

Reviewers

ryanmce
durham
quark

Group Reviewers

Restricted Project

Commits

rFBHGXa1cd7effe38a: pushrebase: check sources of renames when looking for conflicts

Summary

When checking the bundle contents against the revisions it is being rebased
over, include the sources of renames, as changes made in those files also
conflict with the bundle.

Diff Detail

Repository

rFBHGX Facebook Mercurial Extensions

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mbthomas created this revision.Oct 20 2017, 6:10 AM

Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 20 2017, 6:10 AM

mbthomas added a parent revision: D1172: test-pushrebase: add a test case about modifying copy source.Oct 20 2017, 6:10 AM

mbthomas mentioned this in D1172: test-pushrebase: add a test case about modifying copy source.

mbthomas updated this revision to Diff 3046.Oct 20 2017, 6:15 AM

Sweet! Thanks for the quick fix.

This revision is now accepted and ready to land.Oct 20 2017, 3:07 PM

Since this is sensitive to push throughput, we need to be more careful here.

hgext3rd/pushrebase.py
549	This is fast because it just reads the changelog.
552–558	This is not that fast since it looks up file from manifest and extracts the file content.
833	The below (including `_getrevs`) is in a critical section (taking the SQL lock) that affects push throughput. We can pre-calcualte the `renamed` information before entering the critical section, and cache them per revision.

+1 Jun's comment.

This revision now requires changes to proceed.Oct 24 2017, 11:30 AM

mbthomas updated this revision to Diff 3084.Oct 25 2017, 7:14 AM

mbthomas marked 3 inline comments as done.Oct 25 2017, 8:05 AM

LGTM. Thanks for solving this issue!

This looks like it broke test-pushrebase-fastmanifest.t for me?

I fixed the failing test. Looks like it was always a bad test, and this diff just exposed it.

It also broke test-treemanifest-server.t though. Did we run all the tests on this?

Closed by commit rFBHGXa1cd7effe38a: pushrebase: check sources of renames when looking for conflicts (authored by mbthomas). · Explain WhyOct 31 2017, 1:26 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			hgext3rd/pushrebase.py (26 lines)
M			tests/test-pushrebase.t (57 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	3045		Oct 20 2017, 6:10 AM	★	★
Diff 2	3046		Oct 20 2017, 6:15 AM	★	★
Diff 3	3084		Oct 25 2017, 7:14 AM	★	★
Diff 4	3176	rFBHGXa1cd7effe38a8f8a11e8c66a0ace82bbfacd0b5c	Oct 26 2017, 5:47 AM	★	★

	Status	Author	Revision
	Closed	mbthomas	D1199 pushrebase: check sources of renames when looking for conflicts
	Closed	quark	D1172 test-pushrebase: add a test case about modifying copy source

Diff 3176

hgext3rd/pushrebase.py

	os.unlink(bundlefile)			os.unlink(bundlefile)
	except Exception:			except Exception:
	# we would rather see the original exception			# we would rather see the original exception
	pass			pass
	raise			raise

	return bundlefile			return bundlefile

	def _getrevs(bundle, onto):			def _getrenamesrcs(rev):
				'''get all rename sources in a revision'''
				srcs = set()
				revmf = rev.manifest()
				for f in rev.files():
				if f in revmf:
				renamed = rev[f].renamed()
				if renamed:
				srcs.add(renamed[0])
				return srcs

				def _getrevs(bundle, onto, renamesrccache):
	'extracts and validates the revs to be imported'			'extracts and validates the revs to be imported'
	validaterevset(bundle, 'bundle()')			validaterevset(bundle, 'bundle()')
	revs = [bundle[r] for r in bundle.revs('sort(bundle())')]			revs = [bundle[r] for r in bundle.revs('sort(bundle())')]
	onto = bundle[onto.hex()]			onto = bundle[onto.hex()]
	# Fast forward update, no rebase needed			# Fast forward update, no rebase needed
	if list(bundle.set('bundle() & %d::', onto.rev())):			if list(bundle.set('bundle() & %d::', onto.rev())):
	return revs, onto			return revs, onto

	raise error.Abort(_('pushed changesets do not branch from an '			raise error.Abort(_('pushed changesets do not branch from an '
	'ancestor of the desired destination %s')			'ancestor of the desired destination %s')
	% onto.hex())			% onto.hex())
	oldonto = oldonto[0]			oldonto = oldonto[0]

	# Computes a list of all the incoming file changes			# Computes a list of all the incoming file changes
	bundlefiles = set()			bundlefiles = set()
	for bundlerev in revs:			for bundlerev in revs:
	bundlefiles.update(bundlerev.files())			bundlefiles.update(bundlerev.files())
				quarkUnsubmitted Done This is fast because it just reads the changelog. quark: This is fast because it just reads the changelog.

				# Also include sources of renames.
				bundlerevnode = bundlerev.node()
				if bundlerevnode in renamesrccache:
				bundlefiles.update(renamesrccache[bundlerevnode])
				else:
				bundlefiles.update(_getrenamesrcs(bundlerev))

	def findconflicts():			def findconflicts():
				quarkUnsubmitted Done This is not that fast since it looks up file from manifest and extracts the file content. quark: This is not that fast since it looks up file from manifest and extracts the file content.
	# Returns all the files touched in the bundle that are also touched			# Returns all the files touched in the bundle that are also touched
	# between the old onto (ex: our old bookmark location) and the new			# between the old onto (ex: our old bookmark location) and the new
	# onto (ex: the server's actual bookmark location).			# onto (ex: the server's actual bookmark location).
	filematcher = scmutil.matchfiles(bundle, bundlefiles)			filematcher = scmutil.matchfiles(bundle, bundlefiles)
	return onto.manifest().diff(oldonto.manifest(), filematcher).keys()			return onto.manifest().diff(oldonto.manifest(), filematcher).keys()

	def findconflictsfast():			def findconflictsfast():
	# Fast path for detecting conflicting files. Inspects the changelog			# Fast path for detecting conflicting files. Inspects the changelog
	bundle = _createbundlerepo(op, bundlepath)			bundle = _createbundlerepo(op, bundlepath)

	prepushrebasehooks(op, params, bundle, bundlefile)			prepushrebasehooks(op, params, bundle, bundlefile)

	op.repo.ui.setconfig('pushrebase', pushrebasemarker, True)			op.repo.ui.setconfig('pushrebase', pushrebasemarker, True)

	bundlerepocache, preontocache = prefetchcaches(op, params, bundle)			bundlerepocache, preontocache = prefetchcaches(op, params, bundle)

				# Create a cache of rename sources while we don't have the lock.
				renamesrccache = {bundle[r].node(): _getrenamesrcs(bundle[r])
				for r in bundle.revs('bundle()')}

	# Opening the transaction takes the lock, so do it after prepushrebase			# Opening the transaction takes the lock, so do it after prepushrebase
	# and after we've fetched all the cache information we'll need.			# and after we've fetched all the cache information we'll need.
	tr = op.gettransaction()			tr = op.gettransaction()
				quarkUnsubmitted Done The below (including `_getrevs`) is in a critical section (taking the SQL lock) that affects push throughput. We can pre-calcualte the `renamed` information before entering the critical section, and cache them per revision. quark: The below (including `_getrevs`) is in a critical section (taking the SQL lock) that affects…
	hookargs = dict(tr.hookargs)			hookargs = dict(tr.hookargs)

	# Recreate the bundle repo, since taking the lock in gettransaction()			# Recreate the bundle repo, since taking the lock in gettransaction()
	# may have caused it to become out of date.			# may have caused it to become out of date.
	# (but grab a copy of the cache first)			# (but grab a copy of the cache first)
	bundle.close()			bundle.close()
	bundle = _createbundlerepo(op, bundlepath)			bundle = _createbundlerepo(op, bundlepath)

	prefillcaches(op, bundle, bundlerepocache)			prefillcaches(op, bundle, bundlerepocache)

	onto = getontotarget(op, params, bundle)			onto = getontotarget(op, params, bundle)

	revs, oldonto = _getrevs(bundle, onto)			revs, oldonto = _getrevs(bundle, onto, renamesrccache)

	op.repo.hook("prechangegroup", **hookargs)			op.repo.hook("prechangegroup", **hookargs)

	printpushmessage(op, revs, bundle)			printpushmessage(op, revs, bundle)

	# Prepopulate the revlog _cache with the original onto's fulltext. This			# Prepopulate the revlog _cache with the original onto's fulltext. This
	# means reading the new onto's manifest will likely have a much shorter			# means reading the new onto's manifest will likely have a much shorter
	# delta chain to traverse.			# delta chain to traverse.

tests/test-pushrebase.t

	> evolution = all			> evolution = all
	> [paths]			> [paths]
	> default = ../server2			> default = ../server2
	> EOF			> EOF

	$ hg push -r . --to default			$ hg push -r . --to default
	pushing to $TESTTMP/server2 (glob)			pushing to $TESTTMP/server2 (glob)
	searching for changes			searching for changes
	pushing 1 changeset:			abort: conflicting changes in:
	40d149b24655 Copy A to B			A
	2 new changesets from the server will be downloaded
	adding changesets
	adding manifests
	adding file changes
	added 2 changesets with 2 changes to 2 files (+1 heads)
	1 new obsolescence markers
	obsoleted 1 changesets
	2 files updated, 0 files merged, 0 files removed, 0 files unresolved

	SUBOPTIMAL: The pushrebased changest lost the content of "Modify A":

	$ hg log -Gr 'all()' -p --config diff.git=1			[255]
	@ changeset: 3:b0bbcfeb5f8d
	\| tag: tip
	\| user: test
	\| date: Thu Jan 01 00:00:00 1970 +0000
	\| summary: Copy A to B
	\|
	\| diff --git a/A b/B
	\| copy from A
	\| copy to B
	\| --- a/A
	\| +++ b/B
	\| @@ -1,2 +1,1 @@
	\| 1
	\| -2
	\|
	o changeset: 2:986e0f3a27f2
	\| parent: 0:a18fe624bf77
	\| user: test
	\| date: Thu Jan 01 00:00:00 1970 +0000
	\| summary: Modify A
	\|
	\| diff --git a/A b/A
	\| --- a/A
	\| +++ b/A
	\| @@ -1,1 +1,2 @@
	\| 1
	\| +2
	\|
	o changeset: 0:a18fe624bf77
	user: test
	date: Thu Jan 01 00:00:00 1970 +0000
	summary: A

	diff --git a/A b/A
	new file mode 100644
	--- /dev/null
	+++ b/A
	@@ -0,0 +1,1 @@
	+1