This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8879

merge: replace use of actions dict with mergeresult object
ClosedPublic

Authored by pulkit on Aug 5 2020, 4:52 AM.

Download Raw Diff

Details

Reviewers

indygreg

Group Reviewers

hg-reviewers

Commits

rHG5ce63ee1fe3d: merge: replace use of actions dict with mergeresult object

Summary

There are still some places which can be improved by having a dedicated API,
this patch for now make all users of actions dict go through the mergeresult
object API.

Diff Detail

Repository

rHG Mercurial

Lint

Automatic diff as part of commit; lint not applicable.

Unit

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

Event Timeline

pulkit created this revision.Aug 5 2020, 4:52 AM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptAug 5 2020, 4:52 AM

Herald added a subscriber: mercurial-patches. · View Herald Transcript

pulkit added a child revision: D8880: mergeresult: implement a len() function and use it.Aug 5 2020, 4:52 AM

pulkit updated this revision to Diff 22301.Aug 6 2020, 7:14 AM

The introduction of sorting here is interesting. I guess we never sorted filenames before performing working directory modifications before? But surely there is sorting happening elsewhere because anything with ui output would need to be sorted to ensure determinism. And surely tests would have caught that.

Also, I remember there being an interplay between parallel workers and filesystem performance. Something about how file I/O is scheduled with respect to discrete directories. @durin42 might know more. (I remember mpm mentioning this when I was poking around with parallel workers and working directory operations several years ago.)

mercurial/merge.py
1413	The private attribute access here isn't great. But whatever.

This revision is now accepted and ready to land.Aug 8 2020, 12:22 PM

pulkit added a commit: rHG5ce63ee1fe3d: merge: replace use of actions dict with mergeresult object.Aug 8 2020, 12:55 PM

Closed by commit rHG5ce63ee1fe3d: merge: replace use of actions dict with mergeresult object (authored by pulkit). · Explain Why

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			mercurial/merge.py (82 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	22260		Aug 5 2020, 4:52 AM	★	★
Diff 2	22301		Aug 6 2020, 7:14 AM	★	★
Diff 3	22337	rHG5ce63ee1fe3df9aafd62f13c6e179f22bd1a583d	Aug 3 2020, 8:02 AM	★	★

Status	Author	Revision
Closed	pulkit	D8904 merge: drop commitinfo argument to applyupdates (API)
Closed	pulkit	D8903 merge: remove emptyactions() and use collections.defaultdict(list) instead
Closed	pulkit	D8889 merge: pass mergeresult obj in _forgetremoved() (API)
Closed	pulkit	D8888 mergeresult: introduce filemap() which yields filename based mapping
Closed	pulkit	D8887 mergeresult: add `files()` and use it
Closed	pulkit	D8886 mergeresult: introduce getfile() and use it where required
Closed	pulkit	D8885 merge: use ACTION_* constants instead of values in _filternarrowactions()
Closed	pulkit	D8884 merge: rework iteration over mergeresult object in checkpathconflicts()
Closed	pulkit	D8883 applyupdates: simplfy calculation of number of updated files
Closed	pulkit	D8882 mergeresult: yield from getactions() instead of buidling a list and returning
Closed	pulkit	D8881 sparse: replace merge action values with mergestate.ACTION_* constants
Closed	pulkit	D8880 mergeresult: implement a len() function and use it
Closed	pulkit	D8879 merge: replace use of actions dict with mergeresult object
Closed	pulkit	D8878 mergeresult: add sort argument to getactions() method
Closed	pulkit	D8877 merge: pass mergeresult obj in merge._prefetchfiles()
Closed	pulkit	D8876 merge: pass mergeresult obj instead of actions in applyupdates() (API)
Closed	pulkit	D8875 merge: pass mergeresult obj instead of actions in _checkcollision() (API)

Diff 22337

mercurial/merge.py

	(probably mergestate) so that it can be used at commit time.			(probably mergestate) so that it can be used at commit time.

	Return a tuple of (counts, filedata), where counts is a tuple			Return a tuple of (counts, filedata), where counts is a tuple
	(updated, merged, removed, unresolved) that describes how many			(updated, merged, removed, unresolved) that describes how many
	files were affected by the update, and filedata is as described in			files were affected by the update, and filedata is as described in
	batchget.			batchget.
	"""			"""

	actions = mresult.actionsdict
	_prefetchfiles(repo, mctx, mresult)			_prefetchfiles(repo, mctx, mresult)

	updated, merged, removed = 0, 0, 0			updated, merged, removed = 0, 0, 0
	ms = mergestatemod.mergestate.clean(			ms = mergestatemod.mergestate.clean(
	repo, wctx.p1().node(), mctx.node(), labels			repo, wctx.p1().node(), mctx.node(), labels
	)			)

	if commitinfo is None:			if commitinfo is None:
	commitinfo = {}			commitinfo = {}

	for f, op in pycompat.iteritems(commitinfo):			for f, op in pycompat.iteritems(commitinfo):
	# the other side of filenode was choosen while merging, store this in			# the other side of filenode was choosen while merging, store this in
	# mergestate so that it can be reused on commit			# mergestate so that it can be reused on commit
	if op == b'other':			if op == b'other':
	ms.addmergedother(f)			ms.addmergedother(f)

	moves = []			moves = []
	for m, l in actions.items():
	l.sort()

	# 'cd' and 'dc' actions are treated like other merge conflicts			# 'cd' and 'dc' actions are treated like other merge conflicts
	mergeactions = sorted(actions[mergestatemod.ACTION_CHANGED_DELETED])			mergeactions = mresult.getactions(
	mergeactions.extend(sorted(actions[mergestatemod.ACTION_DELETED_CHANGED]))			[
	mergeactions.extend(actions[mergestatemod.ACTION_MERGE])			mergestatemod.ACTION_CHANGED_DELETED,
				mergestatemod.ACTION_DELETED_CHANGED,
				mergestatemod.ACTION_MERGE,
				],
				sort=True,
				)
	for f, args, msg in mergeactions:			for f, args, msg in mergeactions:
	f1, f2, fa, move, anc = args			f1, f2, fa, move, anc = args
	if f == b'.hgsubstate': # merged internally			if f == b'.hgsubstate': # merged internally
	continue			continue
	if f1 is None:			if f1 is None:
	fcl = filemerge.absentfilectx(wctx, fa)			fcl = filemerge.absentfilectx(wctx, fa)
	else:			else:
	repo.ui.debug(b" preserving %s for resolve of %s\n" % (f1, f))			repo.ui.debug(b" preserving %s for resolve of %s\n" % (f1, f))

	# remove renamed files after safely stored			# remove renamed files after safely stored
	for f in moves:			for f in moves:
	if wctx[f].lexists():			if wctx[f].lexists():
	repo.ui.debug(b"removing %s\n" % f)			repo.ui.debug(b"removing %s\n" % f)
	wctx[f].audit()			wctx[f].audit()
	wctx[f].remove()			wctx[f].remove()

	numupdates = sum(			numupdates = len(mresult.actions) - len(
	len(l) for m, l in actions.items() if m != mergestatemod.ACTION_KEEP			mresult._actionmapping[mergestatemod.ACTION_KEEP]
				indygregUnsubmitted Not Done The private attribute access here isn't great. But whatever. indygreg: The private attribute access here isn't great. But whatever.
	)			)
	progress = repo.ui.makeprogress(			progress = repo.ui.makeprogress(
	_(b'updating'), unit=_(b'files'), total=numupdates			_(b'updating'), unit=_(b'files'), total=numupdates
	)			)

	if [			if b'.hgsubstate' in mresult._actionmapping[mergestatemod.ACTION_REMOVE]:
	a
	for a in actions[mergestatemod.ACTION_REMOVE]
	if a[0] == b'.hgsubstate'
	]:
	subrepoutil.submerge(repo, wctx, mctx, wctx, overwrite, labels)			subrepoutil.submerge(repo, wctx, mctx, wctx, overwrite, labels)

	# record path conflicts			# record path conflicts
	for f, args, msg in actions[mergestatemod.ACTION_PATH_CONFLICT]:			for f, args, msg in mresult.getactions(
				[mergestatemod.ACTION_PATH_CONFLICT], sort=True
				):
	f1, fo = args			f1, fo = args
	s = repo.ui.status			s = repo.ui.status
	s(			s(
	_(			_(
	b"%s: path conflict - a file or link has the same name as a "			b"%s: path conflict - a file or link has the same name as a "
	b"directory\n"			b"directory\n"
	)			)
	% f			% f
	cost = 0 if wctx.isinmemory() else 0.001			cost = 0 if wctx.isinmemory() else 0.001

	# remove in parallel (must come before resolving path conflicts and getting)			# remove in parallel (must come before resolving path conflicts and getting)
	prog = worker.worker(			prog = worker.worker(
	repo.ui,			repo.ui,
	cost,			cost,
	batchremove,			batchremove,
	(repo, wctx),			(repo, wctx),
	actions[mergestatemod.ACTION_REMOVE],			mresult.getactions([mergestatemod.ACTION_REMOVE], sort=True),
	)			)
	for i, item in prog:			for i, item in prog:
	progress.increment(step=i, item=item)			progress.increment(step=i, item=item)
	removed = len(actions[mergestatemod.ACTION_REMOVE])			removed = len(mresult._actionmapping[mergestatemod.ACTION_REMOVE])

	# resolve path conflicts (must come before getting)			# resolve path conflicts (must come before getting)
	for f, args, msg in actions[mergestatemod.ACTION_PATH_CONFLICT_RESOLVE]:			for f, args, msg in mresult.getactions(
				[mergestatemod.ACTION_PATH_CONFLICT_RESOLVE], sort=True
				):
	repo.ui.debug(b" %s: %s -> pr\n" % (f, msg))			repo.ui.debug(b" %s: %s -> pr\n" % (f, msg))
	(f0, origf0) = args			(f0, origf0) = args
	if wctx[f0].lexists():			if wctx[f0].lexists():
	repo.ui.note(_(b"moving %s to %s\n") % (f0, f))			repo.ui.note(_(b"moving %s to %s\n") % (f0, f))
	wctx[f].audit()			wctx[f].audit()
	wctx[f].write(wctx.filectx(f0).data(), wctx.filectx(f0).flags())			wctx[f].write(wctx.filectx(f0).data(), wctx.filectx(f0).flags())
	wctx[f0].remove()			wctx[f0].remove()
	progress.increment(item=f)			progress.increment(item=f)

	# get in parallel.			# get in parallel.
	threadsafe = repo.ui.configbool(			threadsafe = repo.ui.configbool(
	b'experimental', b'worker.wdir-get-thread-safe'			b'experimental', b'worker.wdir-get-thread-safe'
	)			)
	prog = worker.worker(			prog = worker.worker(
	repo.ui,			repo.ui,
	cost,			cost,
	batchget,			batchget,
	(repo, mctx, wctx, wantfiledata),			(repo, mctx, wctx, wantfiledata),
	actions[mergestatemod.ACTION_GET],			mresult.getactions([mergestatemod.ACTION_GET], sort=True),
	threadsafe=threadsafe,			threadsafe=threadsafe,
	hasretval=True,			hasretval=True,
	)			)
	getfiledata = {}			getfiledata = {}
	for final, res in prog:			for final, res in prog:
	if final:			if final:
	getfiledata = res			getfiledata = res
	else:			else:
	i, item = res			i, item = res
	progress.increment(step=i, item=item)			progress.increment(step=i, item=item)
	updated = len(actions[mergestatemod.ACTION_GET])			updated = len(mresult._actionmapping[mergestatemod.ACTION_GET])

	if [a for a in actions[mergestatemod.ACTION_GET] if a[0] == b'.hgsubstate']:			if b'.hgsubstate' in mresult._actionmapping[mergestatemod.ACTION_GET]:
	subrepoutil.submerge(repo, wctx, mctx, wctx, overwrite, labels)			subrepoutil.submerge(repo, wctx, mctx, wctx, overwrite, labels)

	# forget (manifest only, just log it) (must come first)			# forget (manifest only, just log it) (must come first)
	for f, args, msg in actions[mergestatemod.ACTION_FORGET]:			for f, args, msg in mresult.getactions(
				(mergestatemod.ACTION_FORGET,), sort=True
				):
	repo.ui.debug(b" %s: %s -> f\n" % (f, msg))			repo.ui.debug(b" %s: %s -> f\n" % (f, msg))
	progress.increment(item=f)			progress.increment(item=f)

	# re-add (manifest only, just log it)			# re-add (manifest only, just log it)
	for f, args, msg in actions[mergestatemod.ACTION_ADD]:			for f, args, msg in mresult.getactions(
				(mergestatemod.ACTION_ADD,), sort=True
				):
	repo.ui.debug(b" %s: %s -> a\n" % (f, msg))			repo.ui.debug(b" %s: %s -> a\n" % (f, msg))
	progress.increment(item=f)			progress.increment(item=f)

	# re-add/mark as modified (manifest only, just log it)			# re-add/mark as modified (manifest only, just log it)
	for f, args, msg in actions[mergestatemod.ACTION_ADD_MODIFIED]:			for f, args, msg in mresult.getactions(
				(mergestatemod.ACTION_ADD_MODIFIED,), sort=True
				):
	repo.ui.debug(b" %s: %s -> am\n" % (f, msg))			repo.ui.debug(b" %s: %s -> am\n" % (f, msg))
	progress.increment(item=f)			progress.increment(item=f)

	# keep (noop, just log it)			# keep (noop, just log it)
	for f, args, msg in actions[mergestatemod.ACTION_KEEP]:			for f, args, msg in mresult.getactions(
				(mergestatemod.ACTION_KEEP,), sort=True
				):
	repo.ui.debug(b" %s: %s -> k\n" % (f, msg))			repo.ui.debug(b" %s: %s -> k\n" % (f, msg))
	# no progress			# no progress

	# directory rename, move local			# directory rename, move local
	for f, args, msg in actions[mergestatemod.ACTION_DIR_RENAME_MOVE_LOCAL]:			for f, args, msg in mresult.getactions(
				(mergestatemod.ACTION_DIR_RENAME_MOVE_LOCAL,), sort=True
				):
	repo.ui.debug(b" %s: %s -> dm\n" % (f, msg))			repo.ui.debug(b" %s: %s -> dm\n" % (f, msg))
	progress.increment(item=f)			progress.increment(item=f)
	f0, flags = args			f0, flags = args
	repo.ui.note(_(b"moving %s to %s\n") % (f0, f))			repo.ui.note(_(b"moving %s to %s\n") % (f0, f))
	wctx[f].audit()			wctx[f].audit()
	wctx[f].write(wctx.filectx(f0).data(), flags)			wctx[f].write(wctx.filectx(f0).data(), flags)
	wctx[f0].remove()			wctx[f0].remove()
	updated += 1			updated += 1

	# local directory rename, get			# local directory rename, get
	for f, args, msg in actions[mergestatemod.ACTION_LOCAL_DIR_RENAME_GET]:			for f, args, msg in mresult.getactions(
				(mergestatemod.ACTION_LOCAL_DIR_RENAME_GET,), sort=True
				):
	repo.ui.debug(b" %s: %s -> dg\n" % (f, msg))			repo.ui.debug(b" %s: %s -> dg\n" % (f, msg))
	progress.increment(item=f)			progress.increment(item=f)
	f0, flags = args			f0, flags = args
	repo.ui.note(_(b"getting %s to %s\n") % (f0, f))			repo.ui.note(_(b"getting %s to %s\n") % (f0, f))
	wctx[f].write(mctx.filectx(f0).data(), flags)			wctx[f].write(mctx.filectx(f0).data(), flags)
	updated += 1			updated += 1

	# exec			# exec
	for f, args, msg in actions[mergestatemod.ACTION_EXEC]:			for f, args, msg in mresult.getactions(
				(mergestatemod.ACTION_EXEC,), sort=True
				):
	repo.ui.debug(b" %s: %s -> e\n" % (f, msg))			repo.ui.debug(b" %s: %s -> e\n" % (f, msg))
	progress.increment(item=f)			progress.increment(item=f)
	(flags,) = args			(flags,) = args
	wctx[f].audit()			wctx[f].audit()
	wctx[f].setflags(b'l' in flags, b'x' in flags)			wctx[f].setflags(b'l' in flags, b'x' in flags)
	updated += 1			updated += 1

	# the ordering is important here -- ms.mergedriver will raise if the merge			# the ordering is important here -- ms.mergedriver will raise if the merge

	msupdated, msmerged, msremoved = ms.counts()			msupdated, msmerged, msremoved = ms.counts()
	updated += msupdated			updated += msupdated
	merged += msmerged			merged += msmerged
	removed += msremoved			removed += msremoved

	extraactions = ms.actions()			extraactions = ms.actions()
	if extraactions:			if extraactions:
	mfiles = {a[0] for a in actions[mergestatemod.ACTION_MERGE]}			mfiles = {
				a[0] for a in mresult.getactions((mergestatemod.ACTION_MERGE,))
				}
	for k, acts in pycompat.iteritems(extraactions):			for k, acts in pycompat.iteritems(extraactions):
	actions[k].extend(acts)
	for a in acts:			for a in acts:
	mresult.addfile(a[0], k, *a[1:])			mresult.addfile(a[0], k, *a[1:])
	if k == mergestatemod.ACTION_GET and wantfiledata:			if k == mergestatemod.ACTION_GET and wantfiledata:
	# no filedata until mergestate is updated to provide it			# no filedata until mergestate is updated to provide it
	for a in acts:			for a in acts:
	getfiledata[a[0]] = None			getfiledata[a[0]] = None
	# Remove these files from actions[ACTION_MERGE] as well. This is			# Remove these files from actions[ACTION_MERGE] as well. This is
	# important because in recordupdates, files in actions[ACTION_MERGE]			# important because in recordupdates, files in actions[ACTION_MERGE]
	# are processed after files in other actions, and the merge driver			# are processed after files in other actions, and the merge driver
	# might add files to those actions via extraactions above. This can			# might add files to those actions via extraactions above. This can
	# lead to a file being recorded twice, with poor results. This is			# lead to a file being recorded twice, with poor results. This is
	# especially problematic for actions[ACTION_REMOVE] (currently only			# especially problematic for actions[ACTION_REMOVE] (currently only
	# possible with the merge driver in the initial merge process;			# possible with the merge driver in the initial merge process;
	# interrupted merges don't go through this flow).			# interrupted merges don't go through this flow).
	#			#
	# The real fix here is to have indexes by both file and action so			# The real fix here is to have indexes by both file and action so
	# that when the action for a file is changed it is automatically			# that when the action for a file is changed it is automatically
	# reflected in the other action lists. But that involves a more			# reflected in the other action lists. But that involves a more
	# complex data structure, so this will do for now.			# complex data structure, so this will do for now.
	#			#
	# We don't need to do the same operation for 'dc' and 'cd' because			# We don't need to do the same operation for 'dc' and 'cd' because
	# those lists aren't consulted again.			# those lists aren't consulted again.
	mfiles.difference_update(a[0] for a in acts)			mfiles.difference_update(a[0] for a in acts)

	actions[mergestatemod.ACTION_MERGE] = [
	a for a in actions[mergestatemod.ACTION_MERGE] if a[0] in mfiles
	]
	for a in mresult.getactions([mergestatemod.ACTION_MERGE]):			for a in mresult.getactions([mergestatemod.ACTION_MERGE]):
	if a[0] not in mfiles:			if a[0] not in mfiles:
	mresult.removefile(a[0])			mresult.removefile(a[0])

	progress.complete()			progress.complete()
	assert len(getfiledata) == (			assert len(getfiledata) == (
	len(actions[mergestatemod.ACTION_GET]) if wantfiledata else 0			len(mresult._actionmapping[mergestatemod.ACTION_GET])
				if wantfiledata
				else 0
	)			)
	return updateresult(updated, merged, removed, unresolved), getfiledata			return updateresult(updated, merged, removed, unresolved), getfiledata


	def _advertisefsmonitor(repo, num_gets, p1node):			def _advertisefsmonitor(repo, num_gets, p1node):
	# Advertise fsmonitor when its presence could be useful.			# Advertise fsmonitor when its presence could be useful.
	#			#
	# We only advertise when performing an update from an empty working			# We only advertise when performing an update from an empty working