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.

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
pulkit updated this revision to Diff 22301.Aug 6 2020, 7:14 AM
indygreg added subscribers: durin42, indygreg.Aug 8 2020, 12:22 PM
indygreg accepted this revision.

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

Revision Contents
Changeset List

PathPackages
Mmercurial/merge.py (82 lines)

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