This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6604

copies: follow copies across merge base without source file (issue6163)
ClosedPublic

Authored by martinvonz on Jul 2 2019, 1:13 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG819712deac69: copies: follow copies across merge base without source file (issue6163)
Summary

As in the previous patch, consider these two histories:

@  4 'rename x to y'
|
o  3 'add x again'
|
o  2 'remove x'
|
| o  1 'modify x'
|/
o  0 'add x'

@  4 'rename x to y'
|
o  3 'add x again'
|
| o  2 'modify x'
| |
| o  1 'add x'
|/
o  0 'base'

We trace copies from the 'modify x' commit to commit 4 by going via
the merge base (commit 0). When tracing file 'y' (_tracefile()) in the
first case, we immediately find the rename from 'x'. We check to see
if 'x' exists in the merge base, which it does, so we consider it a
valid copy. In the second case, 'x' does not exist in the merge base,
so it's not considered a valid copy. As a workaround, this patch makes
it so we also attempt the check in mergecopies's base commit (commit 1
in the second case). That feels pretty ugly to me, but I don't have
any better ideas.

Note that we actually also check not only that the filename matches,
but also that the file's nodeid matches. I don't know why we do that,
but it was like that already before I rewrote mergecopies(). That
means that the rebase will still fail in cases like this (again, it
already failed before my rewrite):

@  4 'rename x to y'
|
o  3 'add x again with content X2'
|
o  2 'remove x'
|
| o  1 'modify x to content X2'
|/
o  1 'modify x to content X1'
|
o  0 'add x with content X0'

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

martinvonz created this revision.Jul 2 2019, 1:13 PM
martinvonz added a commit: rHG819712deac69: copies: follow copies across merge base without source file (issue6163).Jul 10 2019, 3:39 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG819712deac69: copies: follow copies across merge base without source file (issue6163) (authored by martinvonz). · Explain Why
This revision was automatically updated to reflect the committed changes.
yuja added a subscriber: yuja.Jul 11 2019, 8:15 PM
We trace copies from the 'modify x' commit to commit 4 by going via
the merge base (commit 0). When tracing file 'y' (_tracefile()) in the
first case, we immediately find the rename from 'x'. We check to see
if 'x' exists in the merge base, which it does, so we consider it a
valid copy. In the second case, 'x' does not exist in the merge base,
so it's not considered a valid copy. As a workaround, this patch makes
it so we also attempt the check in mergecopies's base commit (commit 1
in the second case). That feels pretty ugly to me, but I don't have
any better ideas.

Maybe better to add some inline doc?

Revision Contents
Changeset List

PathPackages
Mmercurial/copies.py (22 lines)
Mtests/test-copies-unrelated.t (14 lines)

Diff 15875

mercurial/copies.py

t = a.copy() t = a.copy()
for k, v in b.iteritems(): for k, v in b.iteritems():
if v in t: if v in t:
t[k] = t[v] t[k] = t[v]
else: else:
t[k] = v t[k] = v
return t return t
def _tracefile(fctx, am, limit): def _tracefile(fctx, am, basemf, limit):
"""return file context that is the ancestor of fctx present in ancestor """return file context that is the ancestor of fctx present in ancestor
manifest am, stopping after the first ancestor lower than limit""" manifest am, stopping after the first ancestor lower than limit"""
for f in fctx.ancestors(): for f in fctx.ancestors():
path = f.path() path = f.path()
if am.get(path, None) == f.filenode(): if am.get(path, None) == f.filenode():
return path return path
if basemf and basemf.get(path, None) == f.filenode():
return path
if not f.isintroducedafter(limit): if not f.isintroducedafter(limit):
return None return None
def _dirstatecopies(repo, match=None): def _dirstatecopies(repo, match=None):
ds = repo.dirstate ds = repo.dirstate
c = ds.copies().copy() c = ds.copies().copy()
for k in list(c): for k in list(c):
if ds[k] not in 'anm' or (match and not match(k)): if ds[k] not in 'anm' or (match and not match(k)):
mb = b.manifest() mb = b.manifest()
return mb.filesnotin(ma, match=match) return mb.filesnotin(ma, match=match)
def usechangesetcentricalgo(repo): def usechangesetcentricalgo(repo):
"""Checks if we should use changeset-centric copy algorithms""" """Checks if we should use changeset-centric copy algorithms"""
return (repo.ui.config('experimental', 'copies.read-from') in return (repo.ui.config('experimental', 'copies.read-from') in
('changeset-only', 'compatibility')) ('changeset-only', 'compatibility'))
def _committedforwardcopies(a, b, match): def _committedforwardcopies(a, b, base, match):
"""Like _forwardcopies(), but b.rev() cannot be None (working copy)""" """Like _forwardcopies(), but b.rev() cannot be None (working copy)"""
# files might have to be traced back to the fctx parent of the last # files might have to be traced back to the fctx parent of the last
# one-side-only changeset, but not further back than that # one-side-only changeset, but not further back than that
repo = a._repo repo = a._repo
if usechangesetcentricalgo(repo): if usechangesetcentricalgo(repo):
return _changesetforwardcopies(a, b, match) return _changesetforwardcopies(a, b, match)
debug = repo.ui.debugflag and repo.ui.configbool('devel', 'debug.copies') debug = repo.ui.debugflag and repo.ui.configbool('devel', 'debug.copies')
dbg = repo.ui.debug dbg = repo.ui.debug
if debug: if debug:
dbg('debug.copies: looking into rename from %s to %s\n' dbg('debug.copies: looking into rename from %s to %s\n'
% (a, b)) % (a, b))
limit = _findlimit(repo, a, b) limit = _findlimit(repo, a, b)
if debug: if debug:
dbg('debug.copies: search limit: %d\n' % limit) dbg('debug.copies: search limit: %d\n' % limit)
am = a.manifest() am = a.manifest()
basemf = None if base is None else base.manifest()
# find where new files came from # find where new files came from
# we currently don't try to find where old files went, too expensive # we currently don't try to find where old files went, too expensive
# this means we can miss a case like 'hg rm b; hg cp a b' # this means we can miss a case like 'hg rm b; hg cp a b'
cm = {} cm = {}
# Computing the forward missing is quite expensive on large manifests, since # Computing the forward missing is quite expensive on large manifests, since
# it compares the entire manifests. We can optimize it in the common use # it compares the entire manifests. We can optimize it in the common use
for f in sorted(missing): for f in sorted(missing):
if debug: if debug:
dbg('debug.copies: tracing file: %s\n' % f) dbg('debug.copies: tracing file: %s\n' % f)
fctx = b[f] fctx = b[f]
fctx._ancestrycontext = ancestrycontext fctx._ancestrycontext = ancestrycontext
if debug: if debug:
start = util.timer() start = util.timer()
opath = _tracefile(fctx, am, limit) opath = _tracefile(fctx, am, basemf, limit)
if opath: if opath:
if debug: if debug:
dbg('debug.copies: rename of: %s\n' % opath) dbg('debug.copies: rename of: %s\n' % opath)
cm[f] = opath cm[f] = opath
if debug: if debug:
dbg('debug.copies: time: %f seconds\n' dbg('debug.copies: time: %f seconds\n'
% (util.timer() - start)) % (util.timer() - start))
return cm return cm
if childcopies: if childcopies:
newcopies = _chain(newcopies, childcopies) newcopies = _chain(newcopies, childcopies)
for f in childctx.filesremoved(): for f in childctx.filesremoved():
if f in newcopies: if f in newcopies:
del newcopies[f] del newcopies[f]
heapq.heappush(work, (c, parent, newcopies)) heapq.heappush(work, (c, parent, newcopies))
assert False assert False
def _forwardcopies(a, b, match=None): def _forwardcopies(a, b, base=None, match=None):
"""find {dst@b: src@a} copy mapping where a is an ancestor of b""" """find {dst@b: src@a} copy mapping where a is an ancestor of b"""
if base is None:
base = a
match = a.repo().narrowmatch(match) match = a.repo().narrowmatch(match)
# check for working copy # check for working copy
if b.rev() is None: if b.rev() is None:
cm = _committedforwardcopies(a, b.p1(), match) cm = _committedforwardcopies(a, b.p1(), base, match)
# combine copies from dirstate if necessary # combine copies from dirstate if necessary
copies = _chain(cm, _dirstatecopies(b._repo, match)) copies = _chain(cm, _dirstatecopies(b._repo, match))
else: else:
copies = _committedforwardcopies(a, b, match) copies = _committedforwardcopies(a, b, base, match)
return copies return copies
def _backwardrenames(a, b, match): def _backwardrenames(a, b, match):
if a._repo.ui.config('experimental', 'copytrace') == 'off': if a._repo.ui.config('experimental', 'copytrace') == 'off':
return {} return {}
# Even though we're not taking copies into account, 1:n rename situations # Even though we're not taking copies into account, 1:n rename situations
# can still exist (e.g. hg cp a b; hg mv a c). In those cases we # can still exist (e.g. hg cp a b; hg mv a c). In those cases we
copies = _forwardcopies(x, y, match=match) copies = _forwardcopies(x, y, match=match)
elif a == y: elif a == y:
if debug: if debug:
repo.ui.debug('debug.copies: search mode: backward\n') repo.ui.debug('debug.copies: search mode: backward\n')
copies = _backwardrenames(x, y, match=match) copies = _backwardrenames(x, y, match=match)
else: else:
if debug: if debug:
repo.ui.debug('debug.copies: search mode: combined\n') repo.ui.debug('debug.copies: search mode: combined\n')
base = None
if a.rev() != node.nullrev:
base = x
copies = _chain(_backwardrenames(x, a, match=match), copies = _chain(_backwardrenames(x, a, match=match),
_forwardcopies(a, y, match=match)) _forwardcopies(a, y, base, match=match))
_filter(x, y, copies) _filter(x, y, copies)
return copies return copies
def mergecopies(repo, c1, c2, base): def mergecopies(repo, c1, c2, base):
""" """
Finds moves and copies between context c1 and c2 that are relevant for Finds moves and copies between context c1 and c2 that are relevant for
merging. 'base' will be used as the merge base. merging. 'base' will be used as the merge base.

tests/test-copies-unrelated.t

| x | x
| o 2 modify x | o 2 modify x
| | x | | x
| o 1 add x | o 1 add x
|/ x |/ x
o 0 base o 0 base
a a
$ hg debugpathcopies 1 4 $ hg debugpathcopies 1 4
x -> y (no-filelog !) x -> y
#if filelog
BROKEN: This should succeed and merge the changes from x into y
$ hg graft -r 2
grafting 2:* "modify x" (glob)
file 'x' was deleted in local [local] but was modified in other [graft].
What do you want to do?
use (c)hanged version, leave (d)eleted, or leave (u)nresolved? u
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
[255]
#else
$ hg graft -r 2 $ hg graft -r 2
grafting 2:* "modify x" (glob) grafting 2:* "modify x" (glob)
merging y and x to y merging y and x to y
#endif
$ hg co -qC 2 $ hg co -qC 2
$ hg graft -r 4 $ hg graft -r 4
grafting 4:* "rename x to y"* (glob) grafting 4:* "rename x to y"* (glob)
merging x and y to y merging x and y to y
Add x on two branches, with same content but different history, then rename x Add x on two branches, with same content but different history, then rename x
to y on one side. Similar to the case above, here the file's nodeid is to y on one side. Similar to the case above, here the file's nodeid is
different between the branches. different between the branches.