( )⚙ D8799 merge: return a mergeresult obj from manifestmerge(), calculateupdates() (API)

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8799

merge: return a mergeresult obj from manifestmerge(), calculateupdates() (API)
ClosedPublic

Authored by pulkit on Jul 23 2020, 10:11 AM.

Details

Reviewers
indygreg
Group Reviewers
hg-reviewers
Commits
rHG0e18861f96ab: merge: return a mergeresult obj from manifestmerge(), calculateupdates() (API)
Summary

Earlier, manifestmerge() and calculateupdates() returns a tuple of three things.
I wanted to add one more thing to return value.

Introducing a special class which represents results of a merge will help
understand better and also ease adding new return values.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

pulkit created this revision.Jul 23 2020, 10:11 AM
pulkit retitled this revision from merge: return new mergeresult obj from manifestmerge(),calculateupdates() (API) to merge: return a mergeresult obj from manifestmerge(), calculateupdates() (API).Jul 25 2020, 5:56 AM
pulkit updated this revision to Diff 22096.
pulkit added a comment.EditedJul 25 2020, 6:08 AM

This whole series aims to improve the merge code by introducing a mergeresult object which represents result of a merge and using it in various merge related functions. Right now all of this done using actions dict which is also different in different functions. Having a dedicated object will make things easier to understand and do some optimizations.

The series however long consists of small obvious patches.

indygreg accepted this revision.Aug 2 2020, 12:46 PM
indygreg added a subscriber: indygreg.
indygreg added inline comments.
mercurial/merge.py
563

I'm generally not a fan of having trivial @property wrappers for internal attributes like this, as I don't think it adds any value. Especially since there is code mutating the internal state, which is a bit janky.

But I'll approve this anyway. If the code remains unchanged after the completion of the series, my vote is to remove the @property wrappers and access attributes directly.

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

Revision Contents
Changeset List

PathPackages
Mhgext/convert/hg.py (4 lines)
Mhgext/largefiles/overrides.py (50 lines)
Mmercurial/merge.py (85 lines)

Diff 22062

hgext/convert/hg.py

and p2, given that the merge is coming from the given source. and p2, given that the merge is coming from the given source.
This prevents us from losing files that only exist in the target p2 and This prevents us from losing files that only exist in the target p2 and
that don't come from the source repo (like if you're merging multiple that don't come from the source repo (like if you're merging multiple
repositories together). repositories together).
""" """
anc = [p1ctx.ancestor(p2ctx)] anc = [p1ctx.ancestor(p2ctx)]
# Calculate what files are coming from p2 # Calculate what files are coming from p2
actions, diverge, rename = mergemod.calculateupdates( mresult = mergemod.calculateupdates(
self.repo, self.repo,
p1ctx, p1ctx,
p2ctx, p2ctx,
anc, anc,
branchmerge=True, branchmerge=True,
force=True, force=True,
acceptremote=False, acceptremote=False,
followcopies=False, followcopies=False,
) )
for file, (action, info, msg) in pycompat.iteritems(actions): for file, (action, info, msg) in pycompat.iteritems(mresult.actions):
if source.targetfilebelongstosource(file): if source.targetfilebelongstosource(file):
# If the file belongs to the source repo, ignore the p2 # If the file belongs to the source repo, ignore the p2
# since it will be covered by the existing fileset. # since it will be covered by the existing fileset.
continue continue
# If the file requires actual merging, abort. We don't have enough # If the file requires actual merging, abort. We don't have enough
# context to resolve merges correctly. # context to resolve merges correctly.
if action in [b'm', b'dm', b'cd', b'dc']: if action in [b'm', b'dm', b'cd', b'dc']:

hgext/largefiles/overrides.py

# Finally, the merge.applyupdates function will then take care of # Finally, the merge.applyupdates function will then take care of
# writing the files into the working copy and lfcommands.updatelfiles # writing the files into the working copy and lfcommands.updatelfiles
# will update the largefiles. # will update the largefiles.
@eh.wrapfunction(merge, b'calculateupdates') @eh.wrapfunction(merge, b'calculateupdates')
def overridecalculateupdates( def overridecalculateupdates(
origfn, repo, p1, p2, pas, branchmerge, force, acceptremote, *args, **kwargs origfn, repo, p1, p2, pas, branchmerge, force, acceptremote, *args, **kwargs
): ):
overwrite = force and not branchmerge overwrite = force and not branchmerge
actions, diverge, renamedelete = origfn( mresult = origfn(
repo, p1, p2, pas, branchmerge, force, acceptremote, *args, **kwargs repo, p1, p2, pas, branchmerge, force, acceptremote, *args, **kwargs
) )
if overwrite: if overwrite:
return actions, diverge, renamedelete return mresult
# Convert to dictionary with filename as key and action as value. # Convert to dictionary with filename as key and action as value.
lfiles = set() lfiles = set()
for f in actions: for f in mresult.actions:
splitstandin = lfutil.splitstandin(f) splitstandin = lfutil.splitstandin(f)
if splitstandin is not None and splitstandin in p1: if splitstandin is not None and splitstandin in p1:
lfiles.add(splitstandin) lfiles.add(splitstandin)
elif lfutil.standin(f) in p1: elif lfutil.standin(f) in p1:
lfiles.add(f) lfiles.add(f)
for lfile in sorted(lfiles): for lfile in sorted(lfiles):
standin = lfutil.standin(lfile) standin = lfutil.standin(lfile)
(lm, largs, lmsg) = actions.get(lfile, (None, None, None)) (lm, largs, lmsg) = mresult.actions.get(lfile, (None, None, None))
(sm, sargs, smsg) = actions.get(standin, (None, None, None)) (sm, sargs, smsg) = mresult.actions.get(standin, (None, None, None))
if sm in (b'g', b'dc') and lm != b'r': if sm in (b'g', b'dc') and lm != b'r':
if sm == b'dc': if sm == b'dc':
f1, f2, fa, move, anc = sargs f1, f2, fa, move, anc = sargs
sargs = (p2[f2].flags(), False) sargs = (p2[f2].flags(), False)
# Case 1: normal file in the working copy, largefile in # Case 1: normal file in the working copy, largefile in
# the second parent # the second parent
usermsg = ( usermsg = (
_( _(
b'remote turned local normal file %s into a largefile\n' b'remote turned local normal file %s into a largefile\n'
b'use (l)argefile or keep (n)ormal file?' b'use (l)argefile or keep (n)ormal file?'
b'$$ &Largefile $$ &Normal file' b'$$ &Largefile $$ &Normal file'
) )
% lfile % lfile
) )
if repo.ui.promptchoice(usermsg, 0) == 0: # pick remote largefile if repo.ui.promptchoice(usermsg, 0) == 0: # pick remote largefile
actions[lfile] = (b'r', None, b'replaced by standin') mresult.actions[lfile] = (b'r', None, b'replaced by standin')
actions[standin] = (b'g', sargs, b'replaces standin') mresult.actions[standin] = (b'g', sargs, b'replaces standin')
else: # keep local normal file else: # keep local normal file
actions[lfile] = (b'k', None, b'replaces standin') mresult.actions[lfile] = (b'k', None, b'replaces standin')
if branchmerge: if branchmerge:
actions[standin] = (b'k', None, b'replaced by non-standin') mresult.actions[standin] = (
b'k',
None,
b'replaced by non-standin',
)
else: else:
actions[standin] = (b'r', None, b'replaced by non-standin') mresult.actions[standin] = (
b'r',
None,
b'replaced by non-standin',
)
elif lm in (b'g', b'dc') and sm != b'r': elif lm in (b'g', b'dc') and sm != b'r':
if lm == b'dc': if lm == b'dc':
f1, f2, fa, move, anc = largs f1, f2, fa, move, anc = largs
largs = (p2[f2].flags(), False) largs = (p2[f2].flags(), False)
# Case 2: largefile in the working copy, normal file in # Case 2: largefile in the working copy, normal file in
# the second parent # the second parent
usermsg = ( usermsg = (
_( _(
b'remote turned local largefile %s into a normal file\n' b'remote turned local largefile %s into a normal file\n'
b'keep (l)argefile or use (n)ormal file?' b'keep (l)argefile or use (n)ormal file?'
b'$$ &Largefile $$ &Normal file' b'$$ &Largefile $$ &Normal file'
) )
% lfile % lfile
) )
if repo.ui.promptchoice(usermsg, 0) == 0: # keep local largefile if repo.ui.promptchoice(usermsg, 0) == 0: # keep local largefile
if branchmerge: if branchmerge:
# largefile can be restored from standin safely # largefile can be restored from standin safely
actions[lfile] = (b'k', None, b'replaced by standin') mresult.actions[lfile] = (
actions[standin] = (b'k', None, b'replaces standin') b'k',
None,
b'replaced by standin',
)
mresult.actions[standin] = (b'k', None, b'replaces standin')
else: else:
# "lfile" should be marked as "removed" without # "lfile" should be marked as "removed" without
# removal of itself # removal of itself
actions[lfile] = ( mresult.actions[lfile] = (
b'lfmr', b'lfmr',
None, None,
b'forget non-standin largefile', b'forget non-standin largefile',
) )
# linear-merge should treat this largefile as 're-added' # linear-merge should treat this largefile as 're-added'
actions[standin] = (b'a', None, b'keep standin') mresult.actions[standin] = (b'a', None, b'keep standin')
else: # pick remote normal file else: # pick remote normal file
actions[lfile] = (b'g', largs, b'replaces standin') mresult.actions[lfile] = (b'g', largs, b'replaces standin')
actions[standin] = (b'r', None, b'replaced by non-standin') mresult.actions[standin] = (
b'r',
None,
b'replaced by non-standin',
)
return actions, diverge, renamedelete return mresult
@eh.wrapfunction(mergestatemod, b'recordupdates') @eh.wrapfunction(mergestatemod, b'recordupdates')
def mergerecordupdates(orig, repo, actions, branchmerge, getfiledata): def mergerecordupdates(orig, repo, actions, branchmerge, getfiledata):
if b'lfmr' in actions: if b'lfmr' in actions:
lfdirstate = lfutil.openlfdirstate(repo.ui, repo) lfdirstate = lfutil.openlfdirstate(repo.ui, repo)
for lfile, args, msg in actions[b'lfmr']: for lfile, args, msg in actions[b'lfmr']:
# this should be executed before 'orig', to execute 'remove' # this should be executed before 'orig', to execute 'remove'

mercurial/merge.py

hint=_(b'merging in the other direction may work'), hint=_(b'merging in the other direction may work'),
) )
else: else:
raise error.Abort( raise error.Abort(
_(b'conflict in file \'%s\' is outside narrow clone') % f _(b'conflict in file \'%s\' is outside narrow clone') % f
) )
class mergeresult(object):
''''An object representing result of merging manifests.
It has information about what actions need to be performed on dirstate
mapping of divergent renames and other such cases. '''
def __init__(self, actions, diverge, renamedelete):
"""
actions: dict of filename as keys and action related info as values
diverge: mapping of source name -> list of dest name for divergent renames
renamedelete: mapping of source name -> list of destinations for files
deleted on one side and renamed on other.
"""
self._actions = actions
self._diverge = diverge
self._renamedelete = renamedelete
@property
def actions(self):
return self._actions
indygregUnsubmitted
Not Done

I'm generally not a fan of having trivial @property wrappers for internal attributes like this, as I don't think it adds any value. Especially since there is code mutating the internal state, which is a bit janky.

But I'll approve this anyway. If the code remains unchanged after the completion of the series, my vote is to remove the @property wrappers and access attributes directly.

indygreg: I'm generally not a fan of having trivial `@property` wrappers for internal attributes like…
@property
def diverge(self):
return self._diverge
@property
def renamedelete(self):
return self._renamedelete
def setactions(self, actions):
self._actions = actions
def manifestmerge( def manifestmerge(
repo, repo,
wctx, wctx,
p2, p2,
pa, pa,
branchmerge, branchmerge,
force, force,
matcher, matcher,
acceptremote, acceptremote,
followcopies, followcopies,
forcefulldiff=False, forcefulldiff=False,
): ):
""" """
Merge wctx and p2 with ancestor pa and generate merge action list Merge wctx and p2 with ancestor pa and generate merge action list
branchmerge and force are as passed in to update branchmerge and force are as passed in to update
matcher = matcher to filter file lists matcher = matcher to filter file lists
acceptremote = accept the incoming changes without prompting acceptremote = accept the incoming changes without prompting
Returns: Returns an object of mergeresult class
actions: dict of filename as keys and action related info as values
diverge: mapping of source name -> list of dest name for divergent renames
renamedelete: mapping of source name -> list of destinations for files
deleted on one side and renamed on other.
""" """
if matcher is not None and matcher.always(): if matcher is not None and matcher.always():
matcher = None matcher = None
# manifests fetched in order are going to be faster, so prime the caches # manifests fetched in order are going to be faster, so prime the caches
[ [
x.manifest() x.manifest()
for x in sorted(wctx.parents() + [p2, pa], key=scmutil.intrev) for x in sorted(wctx.parents() + [p2, pa], key=scmutil.intrev)
narrowmatch = repo.narrowmatch() narrowmatch = repo.narrowmatch()
if not narrowmatch.always(): if not narrowmatch.always():
# Updates "actions" in place # Updates "actions" in place
_filternarrowactions(narrowmatch, branchmerge, actions) _filternarrowactions(narrowmatch, branchmerge, actions)
renamedelete = branch_copies1.renamedelete renamedelete = branch_copies1.renamedelete
renamedelete.update(branch_copies2.renamedelete) renamedelete.update(branch_copies2.renamedelete)
return actions, diverge, renamedelete return mergeresult(actions, diverge, renamedelete)
def _resolvetrivial(repo, wctx, mctx, ancestor, actions): def _resolvetrivial(repo, wctx, mctx, ancestor, actions):
"""Resolves false conflicts where the nodeid changed but the content """Resolves false conflicts where the nodeid changed but the content
remained the same.""" remained the same."""
# We force a copy of actions.items() because we're going to mutate # We force a copy of actions.items() because we're going to mutate
# actions as we resolve trivial conflicts. # actions as we resolve trivial conflicts.
for f, (m, args, msg) in list(actions.items()): for f, (m, args, msg) in list(actions.items()):
Calculate the actions needed to merge mctx into wctx using ancestors Calculate the actions needed to merge mctx into wctx using ancestors
Uses manifestmerge() to merge manifest and get list of actions required to Uses manifestmerge() to merge manifest and get list of actions required to
perform for merging two manifests. If there are multiple ancestors, uses bid perform for merging two manifests. If there are multiple ancestors, uses bid
merge if enabled. merge if enabled.
Also filters out actions which are unrequired if repository is sparse. Also filters out actions which are unrequired if repository is sparse.
Returns same 3 element tuple as manifestmerge(). Returns mergeresult object same as manifestmerge().
""" """
# Avoid cycle. # Avoid cycle.
from . import sparse from . import sparse
if len(ancestors) == 1: # default if len(ancestors) == 1: # default
actions, diverge, renamedelete = manifestmerge( mresult = manifestmerge(
repo, repo,
wctx, wctx,
mctx, mctx,
ancestors[0], ancestors[0],
branchmerge, branchmerge,
force, force,
matcher, matcher,
acceptremote, acceptremote,
followcopies, followcopies,
) )
_checkunknownfiles(repo, wctx, mctx, force, actions, mergeforce) _checkunknownfiles(repo, wctx, mctx, force, mresult.actions, mergeforce)
else: # only when merge.preferancestor=* - the default else: # only when merge.preferancestor=* - the default
repo.ui.note( repo.ui.note(
_(b"note: merging %s and %s using bids from ancestors %s\n") _(b"note: merging %s and %s using bids from ancestors %s\n")
% ( % (
wctx, wctx,
mctx, mctx,
_(b' and ').join(pycompat.bytestr(anc) for anc in ancestors), _(b' and ').join(pycompat.bytestr(anc) for anc in ancestors),
) )
) )
# Call for bids # Call for bids
fbids = ( fbids = (
{} {}
) # mapping filename to bids (action method to list af actions) ) # mapping filename to bids (action method to list af actions)
diverge, renamedelete = None, None diverge, renamedelete = None, None
for ancestor in ancestors: for ancestor in ancestors:
repo.ui.note(_(b'\ncalculating bids for ancestor %s\n') % ancestor) repo.ui.note(_(b'\ncalculating bids for ancestor %s\n') % ancestor)
actions, diverge1, renamedelete1 = manifestmerge( mresult1 = manifestmerge(
repo, repo,
wctx, wctx,
mctx, mctx,
ancestor, ancestor,
branchmerge, branchmerge,
force, force,
matcher, matcher,
acceptremote, acceptremote,
followcopies, followcopies,
forcefulldiff=True, forcefulldiff=True,
) )
_checkunknownfiles(repo, wctx, mctx, force, actions, mergeforce) _checkunknownfiles(
repo, wctx, mctx, force, mresult1.actions, mergeforce
)
# Track the shortest set of warning on the theory that bid # Track the shortest set of warning on the theory that bid
# merge will correctly incorporate more information # merge will correctly incorporate more information
if diverge is None or len(diverge1) < len(diverge): if diverge is None or len(mresult1.diverge) < len(diverge):
diverge = diverge1 diverge = mresult1.diverge
if renamedelete is None or len(renamedelete) < len(renamedelete1): if renamedelete is None or len(renamedelete) < len(
renamedelete = renamedelete1 mresult1.renamedelete
):
renamedelete = mresult1.renamedelete
for f, a in sorted(pycompat.iteritems(actions)): for f, a in sorted(pycompat.iteritems(mresult1.actions)):
m, args, msg = a m, args, msg = a
if m == mergestatemod.ACTION_GET_OTHER_AND_STORE: if m == mergestatemod.ACTION_GET_OTHER_AND_STORE:
m = mergestatemod.ACTION_GET m = mergestatemod.ACTION_GET
repo.ui.debug(b' %s: %s -> %s\n' % (f, msg, m)) repo.ui.debug(b' %s: %s -> %s\n' % (f, msg, m))
if f in fbids: if f in fbids:
d = fbids[f] d = fbids[f]
if m in d: if m in d:
d[m].append(a) d[m].append(a)
# Pick random action. TODO: Instead, prompt user when resolving # Pick random action. TODO: Instead, prompt user when resolving
m, l = list(bids.items())[0] m, l = list(bids.items())[0]
repo.ui.warn( repo.ui.warn(
_(b' %s: ambiguous merge - picked %s action\n') % (f, m) _(b' %s: ambiguous merge - picked %s action\n') % (f, m)
) )
actions[f] = l[0] actions[f] = l[0]
continue continue
repo.ui.note(_(b'end of auction\n\n')) repo.ui.note(_(b'end of auction\n\n'))
mresult = mergeresult(actions, diverge, renamedelete)
if wctx.rev() is None: if wctx.rev() is None:
fractions = _forgetremoved(wctx, mctx, branchmerge) fractions = _forgetremoved(wctx, mctx, branchmerge)
actions.update(fractions) mresult.actions.update(fractions)
prunedactions = sparse.filterupdatesactions( prunedactions = sparse.filterupdatesactions(
repo, wctx, mctx, branchmerge, actions repo, wctx, mctx, branchmerge, mresult.actions
) )
_resolvetrivial(repo, wctx, mctx, ancestors[0], actions) _resolvetrivial(repo, wctx, mctx, ancestors[0], mresult.actions)
return prunedactions, diverge, renamedelete mresult.setactions(prunedactions)
return mresult
def _getcwd(): def _getcwd():
try: try:
return encoding.getcwd() return encoding.getcwd()
except OSError as err: except OSError as err:
if err.errno == errno.ENOENT: if err.errno == errno.ENOENT:
return None return None
if overwrite: if overwrite:
followcopies = False followcopies = False
elif not pas[0]: elif not pas[0]:
followcopies = False followcopies = False
if not branchmerge and not wc.dirty(missing=True): if not branchmerge and not wc.dirty(missing=True):
followcopies = False followcopies = False
### calculate phase ### calculate phase
actionbyfile, diverge, renamedelete = calculateupdates( mresult = calculateupdates(
repo, repo,
wc, wc,
p2, p2,
pas, pas,
branchmerge, branchmerge,
force, force,
mergeancestor, mergeancestor,
followcopies, followcopies,
matcher=matcher, matcher=matcher,
mergeforce=mergeforce, mergeforce=mergeforce,
) )
actionbyfile = mresult.actions
if updatecheck == UPDATECHECK_NO_CONFLICT: if updatecheck == UPDATECHECK_NO_CONFLICT:
for f, (m, args, msg) in pycompat.iteritems(actionbyfile): for f, (m, args, msg) in pycompat.iteritems(actionbyfile):
if m not in ( if m not in (
mergestatemod.ACTION_GET, mergestatemod.ACTION_GET,
mergestatemod.ACTION_KEEP, mergestatemod.ACTION_KEEP,
mergestatemod.ACTION_EXEC, mergestatemod.ACTION_EXEC,
mergestatemod.ACTION_REMOVE, mergestatemod.ACTION_REMOVE,
mergestatemod.ACTION_PATH_CONFLICT_RESOLVE, mergestatemod.ACTION_PATH_CONFLICT_RESOLVE,
if not branchmerge and ( if not branchmerge and (
force or not wc.dirty(missing=True, branch=False) force or not wc.dirty(missing=True, branch=False)
): ):
_checkcollision(repo, p2.manifest(), None) _checkcollision(repo, p2.manifest(), None)
else: else:
_checkcollision(repo, wc.manifest(), actions) _checkcollision(repo, wc.manifest(), actions)
# divergent renames # divergent renames
for f, fl in sorted(pycompat.iteritems(diverge)): for f, fl in sorted(pycompat.iteritems(mresult.diverge)):
repo.ui.warn( repo.ui.warn(
_( _(
b"note: possible conflict - %s was renamed " b"note: possible conflict - %s was renamed "
b"multiple times to:\n" b"multiple times to:\n"
) )
% f % f
) )
for nf in sorted(fl): for nf in sorted(fl):
repo.ui.warn(b" %s\n" % nf) repo.ui.warn(b" %s\n" % nf)
# rename and delete # rename and delete
for f, fl in sorted(pycompat.iteritems(renamedelete)): for f, fl in sorted(pycompat.iteritems(mresult.renamedelete)):
repo.ui.warn( repo.ui.warn(
_( _(
b"note: possible conflict - %s was deleted " b"note: possible conflict - %s was deleted "
b"and renamed to:\n" b"and renamed to:\n"
) )
% f % f
) )
for nf in sorted(fl): for nf in sorted(fl):