This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8977

merge: check for dir rename dest before adding ACTION_KEEP
Changes PlannedPublic

Authored by pulkit on Sep 2 2020, 7:54 AM.

Details

Reviewers
marmoute
Group Reviewers
hg-reviewers
Summary

A previous patch in the series blindly uses ACTION_KEEP if the file is not
present on both remote and ancestor. This was wrong.

We tries to detect directory renames and in some graft cases, it can be possible
that file is not present on both sides but is created in rename destination of
other directory which exists on remote. In such cases, we need to merge the
rename source from remote with rename dest from local.

This patch makes sure we checks for such rename cases before falling back to
ACTION_KEEP.

Action for rename destination can be added while processing rename destination
or processing rename source. In some cases, when an optimization only diffing
files changes between m2-vs-ma are in play, either of rename dest or rename
source might not be processed.
Hence we need to be extra sure about adding action related to rename
destination.

This issue of missing to check dir rename dest was spotted by a future change
where some tests were failing.

Diff Detail

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

Event Timeline

pulkit created this revision.Sep 2 2020, 7:54 AM
marmoute requested changes to this revision.Sep 11 2020, 8:54 AM
marmoute added a subscriber: marmoute.

Why don't we get a test change with this ?

This revision now requires changes to proceed.Sep 11 2020, 8:54 AM
pulkit added a comment.Sep 14 2020, 4:32 AM
In D8977#135054, @marmoute wrote:

Why don't we get a test change with this ?

The test changes are visible after disabling the m2-vs-ma optimization. I just plugged it before to minimize the test changes in that patch which disables the optimization.

marmoute added a comment.Sep 14 2020, 4:54 AM
In D8977#135146, @pulkit wrote:
In D8977#135054, @marmoute wrote:

Why don't we get a test change with this ?

The test changes are visible after disabling the m2-vs-ma optimization. I just plugged it before to minimize the test changes in that patch which disables the optimization.

Can you mention it in the changeset description ?

pulkit planned changes to this revision.Sep 18 2020, 9:04 AM

I have some changes which need to be made to this patch according to Yuya's review on mailing list.

Revision Contents
Changeset List

PathPackages
Mmercurial/merge.py (67 lines)

Diff 22663

mercurial/merge.py

relevantfiles.add(copykey) relevantfiles.add(copykey)
for movedirkey in branch_copies1.movewithdir: for movedirkey in branch_copies1.movewithdir:
relevantfiles.add(movedirkey) relevantfiles.add(movedirkey)
filesmatcher = scmutil.matchfiles(repo, relevantfiles) filesmatcher = scmutil.matchfiles(repo, relevantfiles)
matcher = matchmod.intersectmatchers(matcher, filesmatcher) matcher = matchmod.intersectmatchers(matcher, filesmatcher)
diff = m1.diff(m2, match=matcher) diff = m1.diff(m2, match=matcher)
# we might miss adding an action for the rename destination file as it
# didn't change between m2 and ma and hence was optimized out. However,
# the file is important as it contains rename information which is
# important. Stores such actions and makes sure they are all there
# in mresult object. If not, adds them.
rename_dest_actions = {}
for f, ((n1, fl1), (n2, fl2)) in pycompat.iteritems(diff): for f, ((n1, fl1), (n2, fl2)) in pycompat.iteritems(diff):
if n1 and n2: # file exists on both local and remote side if n1 and n2: # file exists on both local and remote side
if f not in ma: if f not in ma:
# TODO: what if they're renamed from different sources? # TODO: what if they're renamed from different sources?
fa = branch_copies1.copy.get( fa = branch_copies1.copy.get(
f, None f, None
) or branch_copies2.copy.get(f, None) ) or branch_copies2.copy.get(f, None)
args, msg = None, None args, msg = None, None
mresult.addfile( mresult.addfile(
f, mergestatemod.ACTION_FORGET, None, b'remote deleted', f, mergestatemod.ACTION_FORGET, None, b'remote deleted',
) )
else: else:
mresult.addfile( mresult.addfile(
f, mergestatemod.ACTION_REMOVE, None, b'other deleted', f, mergestatemod.ACTION_REMOVE, None, b'other deleted',
) )
else: # file not in ancestor, not in remote else: # file not in ancestor, not in remote
rename_found = False
for source, dest in branch_copies1.dirmove.items():
if f.startswith(dest):
# this directory in which this file is created
# is moved from somewhere else
# we should check if the source file exists on
# remote side
sf = source + f[len(dest) :]
if sf in m2:
rename_found = True
if m2[sf] == ma[sf]:
mresult.addfile(
f,
mergestatemod.ACTION_KEEP,
None,
b'file is rename dest and rename source'
b' did not change',
)
else:
mresult.addfile(
f,
mergestatemod.ACTION_MERGE,
(f, sf, sf, False, pa.node()),
b'local directory rename - respect move '
b'from %s' % sf,
)
break
if not rename_found:
mresult.addfile( mresult.addfile(
f, f,
mergestatemod.ACTION_KEEP, mergestatemod.ACTION_KEEP,
None, None,
b'ancestor missing, remote missing', b'ancestor missing, remote missing',
) )
elif n2: # file exists only on remote side elif n2: # file exists only on remote side
if f in copied1: if f in copied1:
pass # we'll deal with it on m1 side pass # we'll deal with it on m1 side
elif f in branch_copies2.movewithdir: elif f in branch_copies2.movewithdir:
f2 = branch_copies2.movewithdir[f] f2 = branch_copies2.movewithdir[f]
if f2 in m1: if f2 in m1:
mresult.addfile( mresult.addfile(
elif n2 != ma[f]: elif n2 != ma[f]:
df = None df = None
for d in branch_copies1.dirmove: for d in branch_copies1.dirmove:
if f.startswith(d): if f.startswith(d):
# new file added in a directory that was moved # new file added in a directory that was moved
df = branch_copies1.dirmove[d] + f[len(d) :] df = branch_copies1.dirmove[d] + f[len(d) :]
break break
if df is not None and df in m1: if df is not None and df in m1:
# this file is not present in local however its a source
# of rename for another file. keep this file not
# present in local.
mresult.addfile( mresult.addfile(
df, f,
mergestatemod.ACTION_KEEP_ABSENT,
None,
b'remote directory is rename source - respect move '
b'to %s' % df,
)
# action for rename destination is important and can be
# missed due to some optimizations.
rename_dest_actions[df] = (
mergestatemod.ACTION_MERGE, mergestatemod.ACTION_MERGE,
(df, f, f, False, pa.node()), (df, f, f, False, pa.node()),
b'local directory rename - respect move ' b'local directory rename - respect move '
b'from %s' % f, b'from %s' % f,
) )
elif acceptremote: elif acceptremote:
mresult.addfile( mresult.addfile(
f, f,
else: else:
mresult.addfile( mresult.addfile(
f, f,
mergestatemod.ACTION_KEEP_ABSENT, mergestatemod.ACTION_KEEP_ABSENT,
None, None,
b'local not present, remote unchanged', b'local not present, remote unchanged',
) )
# make sure actions for rename destination are there and if not
# add them
for f, data in pycompat.iteritems(rename_dest_actions):
if mresult.getfile(f) is None:
mresult.addfile(f, *data)
if repo.ui.configbool(b'experimental', b'merge.checkpathconflicts'): if repo.ui.configbool(b'experimental', b'merge.checkpathconflicts'):
# If we are merging, look for path conflicts. # If we are merging, look for path conflicts.
checkpathconflicts(repo, wctx, p2, mresult) checkpathconflicts(repo, wctx, p2, mresult)
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, mresult) _filternarrowactions(narrowmatch, branchmerge, mresult)