Patchwork [2,of,2,v2] convert: keep converted hg parents that are outside convert.hg.revs (BC)

login
register
mail settings
Submitter Mads Kiilerich
Date April 12, 2016, 10:18 p.m.
Message ID <f7c245a1e550915fca41.1460499487@madski>
Download mbox | patch
Permalink /patch/14568/
State Accepted
Delegated to: Yuya Nishihara
Headers show

Comments

Mads Kiilerich - April 12, 2016, 10:18 p.m.
# HG changeset patch
# User Mads Kiilerich <madski@unity3d.com>
# Date 1460499381 -7200
#      Wed Apr 13 00:16:21 2016 +0200
# Node ID f7c245a1e550915fca416e76abff626af85e4291
# Parent  637869e31c974eecea10853fadac216107c434e1
convert: keep converted hg parents that are outside convert.hg.revs (BC)

Before, when converting revisions without also including their already
converted parents in convert.hg.revs, the parents would no longer be parents.

That seems unfortunate and we dare to assume that nobody ever wants that.

Instead, preserve parents that are outside the current convert range but
already have been converted.

The parents returned in getcommit() are unconditionally converted, so we
introduce a separate optparents with optional parents.
Sean Farley - April 12, 2016, 10:23 p.m.
Mads Kiilerich <mads@kiilerich.com> writes:

> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1460499381 -7200
> #      Wed Apr 13 00:16:21 2016 +0200
> # Node ID f7c245a1e550915fca416e76abff626af85e4291
> # Parent  637869e31c974eecea10853fadac216107c434e1
> convert: keep converted hg parents that are outside convert.hg.revs (BC)
>
> Before, when converting revisions without also including their already
> converted parents in convert.hg.revs, the parents would no longer be parents.
>
> That seems unfortunate and we dare to assume that nobody ever wants that.
>
> Instead, preserve parents that are outside the current convert range but
> already have been converted.
>
> The parents returned in getcommit() are unconditionally converted, so we
> introduce a separate optparents with optional parents.

Reading the code, this seems like a pretty straight-forward fix to me.
Perhaps a little extra overhead but that seems negligible here.
Yuya Nishihara - April 13, 2016, 3:49 p.m.
On Wed, 13 Apr 2016 00:18:07 +0200, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1460499381 -7200
> #      Wed Apr 13 00:16:21 2016 +0200
> # Node ID f7c245a1e550915fca416e76abff626af85e4291
> # Parent  637869e31c974eecea10853fadac216107c434e1
> convert: keep converted hg parents that are outside convert.hg.revs (BC)

Yeah, this looks much better than V1, thanks.

> -        parents = [p.hex() for p in self._parents(ctx)]
> +        _parents = self._parents(ctx)
> +        parents = [p.hex() for p in _parents]
> +        optparents = [p.hex() for p in ctx.parents() if p not in _parents]

One nit. ctx.parents() may contain null revision if p1 is null. Perhaps we'd
better filter out it like self._parents().

If you agree, I'll change s/if p not in _parents/if p and p not in _parents/
in flight.
Mads Kiilerich - April 13, 2016, 11:07 p.m.
On 04/13/2016 05:49 PM, Yuya Nishihara wrote:
>> -        parents = [p.hex() for p in self._parents(ctx)]
>> +        _parents = self._parents(ctx)
>> +        parents = [p.hex() for p in _parents]
>> +        optparents = [p.hex() for p in ctx.parents() if p not in _parents]
> One nit. ctx.parents() may contain null revision if p1 is null. Perhaps we'd
> better filter out it like self._parents().
>
> If you agree, I'll change s/if p not in _parents/if p and p not in _parents/
> in flight.

Yeah, seems reasonable. Thanks!

(It would be weird if the null revision has been converted, so I doubt 
it makes any actual difference ...)

/Mads
Yuya Nishihara - April 14, 2016, 1:28 p.m.
On Thu, 14 Apr 2016 01:07:31 +0200, Mads Kiilerich wrote:
> On 04/13/2016 05:49 PM, Yuya Nishihara wrote:
> >> -        parents = [p.hex() for p in self._parents(ctx)]
> >> +        _parents = self._parents(ctx)
> >> +        parents = [p.hex() for p in _parents]
> >> +        optparents = [p.hex() for p in ctx.parents() if p not in _parents]  
> > One nit. ctx.parents() may contain null revision if p1 is null. Perhaps we'd
> > better filter out it like self._parents().
> >
> > If you agree, I'll change s/if p not in _parents/if p and p not in _parents/
> > in flight.  
> 
> Yeah, seems reasonable. Thanks!
> 
> (It would be weird if the null revision has been converted, so I doubt 
> it makes any actual difference ...)

There's no practical difference as optparets is filtered by converter.map.

Pushed the modified version, thanks.

Patch

diff --git a/hgext/convert/common.py b/hgext/convert/common.py
--- a/hgext/convert/common.py
+++ b/hgext/convert/common.py
@@ -55,11 +55,13 @@  SKIPREV = 'SKIP'
 
 class commit(object):
     def __init__(self, author, date, desc, parents, branch=None, rev=None,
-                 extra={}, sortkey=None, saverev=True, phase=phases.draft):
+                 extra={}, sortkey=None, saverev=True, phase=phases.draft,
+                 optparents=None):
         self.author = author or 'unknown'
         self.date = date or '0 0'
         self.desc = desc
-        self.parents = parents
+        self.parents = parents # will be converted and used as parents
+        self.optparents = optparents or [] # will be used if already converted
         self.branch = branch
         self.rev = rev
         self.extra = extra
diff --git a/hgext/convert/convcmd.py b/hgext/convert/convcmd.py
--- a/hgext/convert/convcmd.py
+++ b/hgext/convert/convcmd.py
@@ -472,6 +472,9 @@  class converter(object):
             parents = [self.map.get(p, p) for p in parents]
         except KeyError:
             parents = [b[0] for b in pbranches]
+            parents.extend(self.map[x]
+                           for x in commit.optparents
+                           if x in self.map)
         if len(pbranches) != 2:
             cleanp2 = set()
         if len(parents) < 3:
diff --git a/hgext/convert/hg.py b/hgext/convert/hg.py
--- a/hgext/convert/hg.py
+++ b/hgext/convert/hg.py
@@ -582,7 +582,9 @@  class mercurial_source(common.converter_
 
     def getcommit(self, rev):
         ctx = self._changectx(rev)
-        parents = [p.hex() for p in self._parents(ctx)]
+        _parents = self._parents(ctx)
+        parents = [p.hex() for p in _parents]
+        optparents = [p.hex() for p in ctx.parents() if p not in _parents]
         crev = rev
 
         return common.commit(author=ctx.user(),
@@ -591,6 +593,7 @@  class mercurial_source(common.converter_
                              desc=ctx.description(),
                              rev=crev,
                              parents=parents,
+                             optparents=optparents,
                              branch=ctx.branch(),
                              extra=ctx.extra(),
                              sortkey=ctx.rev(),
diff --git a/tests/test-convert-hg-startrev.t b/tests/test-convert-hg-startrev.t
--- a/tests/test-convert-hg-startrev.t
+++ b/tests/test-convert-hg-startrev.t
@@ -222,7 +222,7 @@  Convert from specified revs
   o  0 "0: add a b f" files: a b f
   
 Convert in multiple steps that doesn't overlap - the link to the parent is
-currently missing
+preserved anyway
 
   $ hg convert --config convert.hg.revs=::1 source multistep
   initializing destination multistep repository
@@ -237,8 +237,8 @@  currently missing
   converting...
   0 2: copy e from a, change b
   $ glog multistep
-  o  2 "2: copy e from a, change b" files: a b c d e
-  
+  o  2 "2: copy e from a, change b" files: b e
+  |
   o  1 "1: add c, move f to d" files: c d f
   |
   o  0 "0: add a b f" files: a b f