Submitter | Siddharth Agarwal |
---|---|
Date | Dec. 21, 2012, 9:08 p.m. |
Message ID | <078b88a00bff61b44f10.1356124101@sid0x220> |
Download | mbox | patch |
Permalink | /patch/262/ |
State | Accepted |
Commit | 6c35b53cd28be0bfa8dd7cb2c54dec62ba73bafb |
Headers | show |
Comments
On 12/21/2012 01:08 PM, Siddharth Agarwal wrote: > diff --git a/mercurial/copies.py b/mercurial/copies.py > --- a/mercurial/copies.py > +++ b/mercurial/copies.py > @@ -170,11 +170,17 @@ def mergecopies(repo, c1, c2, ca): > Find moves and copies between context c1 and c2 that are relevant > for merging. > > - Returns two dicts, "copy" and "diverge". > + Returns four dicts: "copy", "moveviadirmove", "diverge", and > + "renamedelete". > BTW I know the name "moveviadirmove" is not a very good one, but I couldn't think of a better one. Suggestions welcome.
On 21 d?c. 2012, at 22:08, Siddharth Agarwal wrote: > # HG changeset patch > # User Siddharth Agarwal <sid0 at fb.com> > # Date 1356044912 28800 > # Node ID 078b88a00bff61b44f10d71881a95324bf787925 > # Parent d00f0bfe007da572898cb37305a70ec31ee1ce8c > copies: separate moves via directory renames from explicit copies > > Currently the "copy" dict contains both explicit copies/moves made by a > context and pending moves that need to happen because the other context moved > the directory the file was in. For explicit copies, the dict stores a > destination to source map, while for pending moves via directory renames, it > stores a source to destination map. The merge code uses this fact in a non- > obvious way to differentiate between these two cases. > > We make this explicit by storing these pending moves in a separate dict. The > dict still has a source to destination map, but that is called out in the > docstring. > > diff --git a/mercurial/copies.py b/mercurial/copies.py > --- a/mercurial/copies.py > +++ b/mercurial/copies.py > @@ -170,11 +170,17 @@ def mergecopies(repo, c1, c2, ca): > Find moves and copies between context c1 and c2 that are relevant > for merging. > > - Returns two dicts, "copy" and "diverge". > + Returns four dicts: "copy", "moveviadirmove", "diverge", and > + "renamedelete". I would use movewithdit instead of moveviadirmove > > "copy" is a mapping from destination name -> source name, > where source is in c1 and destination is in c2 or vice-versa. > > + "moveviadirmove" is a mapping from source name -> destination name, > + where the file at source present in one context but not the other > + needs to be moved to destination by the merge process, because the > + other context moved the directory it is in. > + > "diverge" is a mapping of source name -> list of destination names > for divergent renames. > > @@ -183,16 +189,16 @@ def mergecopies(repo, c1, c2, ca): > """ > # avoid silly behavior for update from empty dir > if not c1 or not c2 or c1 == c2: > - return {}, {}, {} > + return {}, {}, {}, {} > > # avoid silly behavior for parent -> working dir > if c2.node() is None and c1.node() == repo.dirstate.p1(): > - return repo.dirstate.copies(), {}, {} > + return repo.dirstate.copies(), {}, {}, {} > > limit = _findlimit(repo, c1.rev(), c2.rev()) > if limit is None: > # no common ancestor, no copies > - return {}, {}, {} > + return {}, {}, {}, {} > m1 = c1.manifest() > m2 = c2.manifest() > ma = ca.manifest() > @@ -206,6 +212,7 @@ def mergecopies(repo, c1, c2, ca): > > ctx = util.lrucachefunc(makectx) > copy = {} > + moveviadirmove = {} > fullcopy = {} > diverge = {} > > @@ -315,7 +322,7 @@ def mergecopies(repo, c1, c2, ca): > del diverge2 > > if not fullcopy: > - return copy, diverge, renamedelete > + return copy, moveviadirmove, diverge, renamedelete > > repo.ui.debug(" checking for directory renames\n") > > @@ -352,7 +359,7 @@ def mergecopies(repo, c1, c2, ca): > del d1, d2, invalid > > if not dirmove: > - return copy, diverge, renamedelete > + return copy, moveviadirmove, diverge, renamedelete > > for d in dirmove: > repo.ui.debug(" dir %s -> %s\n" % (d, dirmove[d])) > @@ -365,8 +372,8 @@ def mergecopies(repo, c1, c2, ca): > # new file added in a directory that was moved, move it > df = dirmove[d] + f[len(d):] > if df not in copy: > - copy[f] = df > - repo.ui.debug(" file %s -> %s\n" % (f, copy[f])) > + moveviadirmove[f] = df > + repo.ui.debug(" file %s -> %s\n" % (f, df)) > break > > - return copy, diverge, renamedelete > + return copy, moveviadirmove, diverge, renamedelete > diff --git a/mercurial/merge.py b/mercurial/merge.py > --- a/mercurial/merge.py > +++ b/mercurial/merge.py > @@ -213,14 +213,15 @@ def manifestmerge(repo, p1, p2, pa, over > repo.ui.debug(" %s: %s -> %s\n" % (f, msg, m)) > action.append((f, m) + args) > > - action, copy = [], {} > + action, copy, moveviadirmove = [], {}, {} > > if overwrite: > pa = p1 > elif pa == p2: # backwards > pa = p1.p1() > elif pa and repo.ui.configbool("merge", "followcopies", True): > - copy, diverge, renamedelete = copies.mergecopies(repo, p1, p2, pa) > + copy, moveviadirmove, diverge, renamedelete = copies.mergecopies( > + repo, p1, p2, pa) You should just cut that in two lines. ret = copies.mergecopies(repo, p1, p2, pa) copy, moveviadirmove, diverge, renamedelete = ret The rest of the patch LGTM. I assume you ran a full test-suite on it.
Patch
diff --git a/mercurial/copies.py b/mercurial/copies.py --- a/mercurial/copies.py +++ b/mercurial/copies.py @@ -170,11 +170,17 @@ def mergecopies(repo, c1, c2, ca): Find moves and copies between context c1 and c2 that are relevant for merging. - Returns two dicts, "copy" and "diverge". + Returns four dicts: "copy", "moveviadirmove", "diverge", and + "renamedelete". "copy" is a mapping from destination name -> source name, where source is in c1 and destination is in c2 or vice-versa. + "moveviadirmove" is a mapping from source name -> destination name, + where the file at source present in one context but not the other + needs to be moved to destination by the merge process, because the + other context moved the directory it is in. + "diverge" is a mapping of source name -> list of destination names for divergent renames. @@ -183,16 +189,16 @@ def mergecopies(repo, c1, c2, ca): """ # avoid silly behavior for update from empty dir if not c1 or not c2 or c1 == c2: - return {}, {}, {} + return {}, {}, {}, {} # avoid silly behavior for parent -> working dir if c2.node() is None and c1.node() == repo.dirstate.p1(): - return repo.dirstate.copies(), {}, {} + return repo.dirstate.copies(), {}, {}, {} limit = _findlimit(repo, c1.rev(), c2.rev()) if limit is None: # no common ancestor, no copies - return {}, {}, {} + return {}, {}, {}, {} m1 = c1.manifest() m2 = c2.manifest() ma = ca.manifest() @@ -206,6 +212,7 @@ def mergecopies(repo, c1, c2, ca): ctx = util.lrucachefunc(makectx) copy = {} + moveviadirmove = {} fullcopy = {} diverge = {} @@ -315,7 +322,7 @@ def mergecopies(repo, c1, c2, ca): del diverge2 if not fullcopy: - return copy, diverge, renamedelete + return copy, moveviadirmove, diverge, renamedelete repo.ui.debug(" checking for directory renames\n") @@ -352,7 +359,7 @@ def mergecopies(repo, c1, c2, ca): del d1, d2, invalid if not dirmove: - return copy, diverge, renamedelete + return copy, moveviadirmove, diverge, renamedelete for d in dirmove: repo.ui.debug(" dir %s -> %s\n" % (d, dirmove[d])) @@ -365,8 +372,8 @@ def mergecopies(repo, c1, c2, ca): # new file added in a directory that was moved, move it df = dirmove[d] + f[len(d):] if df not in copy: - copy[f] = df - repo.ui.debug(" file %s -> %s\n" % (f, copy[f])) + moveviadirmove[f] = df + repo.ui.debug(" file %s -> %s\n" % (f, df)) break - return copy, diverge, renamedelete + return copy, moveviadirmove, diverge, renamedelete diff --git a/mercurial/merge.py b/mercurial/merge.py --- a/mercurial/merge.py +++ b/mercurial/merge.py @@ -213,14 +213,15 @@ def manifestmerge(repo, p1, p2, pa, over repo.ui.debug(" %s: %s -> %s\n" % (f, msg, m)) action.append((f, m) + args) - action, copy = [], {} + action, copy, moveviadirmove = [], {}, {} if overwrite: pa = p1 elif pa == p2: # backwards pa = p1.p1() elif pa and repo.ui.configbool("merge", "followcopies", True): - copy, diverge, renamedelete = copies.mergecopies(repo, p1, p2, pa) + copy, moveviadirmove, diverge, renamedelete = copies.mergecopies( + repo, p1, p2, pa) for of, fl in diverge.iteritems(): act("divergent renames", "dr", of, fl) for of, fl in renamedelete.iteritems(): @@ -233,6 +234,7 @@ def manifestmerge(repo, p1, p2, pa, over m1, m2, ma = p1.manifest(), p2.manifest(), pa.manifest() copied = set(copy.values()) + copied.update(moveviadirmove.values()) if '.hgsubstate' in m1: # check whether sub state is modified @@ -259,14 +261,14 @@ def manifestmerge(repo, p1, p2, pa, over act("versions differ", "m", f, f, f, rflags, False) elif f in copied: # files we'll deal with on m2 side pass - elif f in copy: + elif f in moveviadirmove: # directory rename + f2 = moveviadirmove[f] + act("remote renamed directory to " + f2, "d", f, None, f2, + m1.flags(f)) + elif f in copy: # case 2 A,B/B/B or case 4,21 A/B/B f2 = copy[f] - if f2 not in m2: # directory rename - act("remote renamed directory to " + f2, "d", - f, None, f2, m1.flags(f)) - else: # case 2 A,B/B/B or case 4,21 A/B/B - act("local copied/moved to " + f2, "m", - f, f2, f, fmerge(f, f2, f2), False) + act("local copied/moved to " + f2, "m", f, f2, f, + fmerge(f, f2, f2), False) elif f in ma: # clean, a different, no remote if n != ma[f]: if repo.ui.promptchoice( @@ -286,12 +288,13 @@ def manifestmerge(repo, p1, p2, pa, over continue if f in m1 or f in copied: # files already visited continue - if f in copy: + if f in moveviadirmove: + f2 = moveviadirmove[f] + act("local renamed directory to " + f2, "d", None, f, f2, + m2.flags(f)) + elif f in copy: f2 = copy[f] - if f2 not in m1: # directory rename - act("local renamed directory to " + f2, "d", - None, f, f2, m2.flags(f)) - elif f2 in m2: # rename case 1, A/A,B/A + if f2 in m2: # rename case 1, A/A,B/A act("remote copied to " + f, "m", f2, f, f, fmerge(f2, f, f2), False) else: # case 3,20 A/B/A