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

login
register
mail settings
Submitter Mads Kiilerich
Date March 25, 2016, 8:14 a.m.
Message ID <ede9dd4902ae4a5cd464.1458893643@localhost.localdomain>
Download mbox | patch
Permalink /patch/14065/
State Changes Requested
Delegated to: Yuya Nishihara
Headers show

Comments

Mads Kiilerich - March 25, 2016, 8:14 a.m.
# HG changeset patch
# User Mads Kiilerich <madski@unity3d.com>
# Date 1458860864 25200
#      Thu Mar 24 16:07:44 2016 -0700
# Node ID ede9dd4902ae4a5cd464811532acdcd2fd02728f
# Parent  0dd592daa18caead5b26cfc1f438b5d885d8b8ce
convert: keep converted parents that are outside convert.hg.revs (BC)

Before, when converting revisions without also converting their parents,
parents that already had been converted 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 has been converted.

The parents returned in getcommit() are currently unconditionally converted, so
it doesn't work to return all parents there. Instead, pass the converted
revisions to getcommit() so it freely can return parents that already has been
converted - and thus preserve parents that not are in the current conversion.
timeless - March 25, 2016, 5:03 p.m.
On Fri, Mar 25, 2016 at 4:14 AM, Mads Kiilerich <mads@kiilerich.com> wrote:
> Instead, preserve parents that are outside the current convert range but
> already has been converted.

> it doesn't work to return all parents there. Instead, pass the converted
> revisions to getcommit() so it freely can return parents that already has been

already has => already have (x2)
Yuya Nishihara - March 27, 2016, 4:07 p.m.
On Fri, 25 Mar 2016 01:14:03 -0700, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1458860864 25200
> #      Thu Mar 24 16:07:44 2016 -0700
> # Node ID ede9dd4902ae4a5cd464811532acdcd2fd02728f
> # Parent  0dd592daa18caead5b26cfc1f438b5d885d8b8ce
> convert: keep converted parents that are outside convert.hg.revs (BC)
> 
> Before, when converting revisions without also converting their parents,
> parents that already had been converted 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 has been converted.

I think the direction is good.

> --- a/hgext/convert/filemap.py
> +++ b/hgext/convert/filemap.py
> @@ -259,7 +259,7 @@ class filemap_source(common.converter_so
>      def getheads(self):
>          return self.base.getheads()
>  
> -    def getcommit(self, rev):
> +    def getcommit(self, rev, revmap=None):
>          # We want to save a reference to the commit objects to be able
>          # to rewrite their parents later on.
>          c = self.commits[rev] = self.base.getcommit(rev)

Perhaps filemap_source should forward revmap to base source, but I got
IndexError if I changed it in that way.

Can we pass a copy of the known revmap by constructor? I think it will be
simpler because revmap isn't necessary to be updated dynamically.
Mads Kiilerich - March 27, 2016, 8:23 p.m.
On 03/27/2016 09:07 AM, Yuya Nishihara wrote:
> On Fri, 25 Mar 2016 01:14:03 -0700, Mads Kiilerich wrote:
>> # HG changeset patch
>> # User Mads Kiilerich <madski@unity3d.com>
>> # Date 1458860864 25200
>> #      Thu Mar 24 16:07:44 2016 -0700
>> # Node ID ede9dd4902ae4a5cd464811532acdcd2fd02728f
>> # Parent  0dd592daa18caead5b26cfc1f438b5d885d8b8ce
>> convert: keep converted parents that are outside convert.hg.revs (BC)
>>
>> Before, when converting revisions without also converting their parents,
>> parents that already had been converted 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 has been converted.
> I think the direction is good.
>
>> --- a/hgext/convert/filemap.py
>> +++ b/hgext/convert/filemap.py
>> @@ -259,7 +259,7 @@ class filemap_source(common.converter_so
>>       def getheads(self):
>>           return self.base.getheads()
>>   
>> -    def getcommit(self, rev):
>> +    def getcommit(self, rev, revmap=None):
>>           # We want to save a reference to the commit objects to be able
>>           # to rewrite their parents later on.
>>           c = self.commits[rev] = self.base.getcommit(rev)
> Perhaps filemap_source should forward revmap to base source, but I got
> IndexError if I changed it in that way.
>
> Can we pass a copy of the known revmap by constructor? I think it will be
> simpler because revmap isn't necessary to be updated dynamically.

I don't see a good way to do that with the current convert architecture. 
The source and destination are created independently before both are 
passed to convert().

The same result could also be achieved with other more or less intrusive 
changes to how parents generally are handled or to the conversion 
framework in general. I am not sure which one would be better.

/Mads
Yuya Nishihara - March 28, 2016, 4:34 p.m.
On Sun, 27 Mar 2016 13:23:46 -0700
Mads Kiilerich <mads@kiilerich.com> wrote:

> On 03/27/2016 09:07 AM, Yuya Nishihara wrote:
> > On Fri, 25 Mar 2016 01:14:03 -0700, Mads Kiilerich wrote:  
> >> # HG changeset patch
> >> # User Mads Kiilerich <madski@unity3d.com>
> >> # Date 1458860864 25200
> >> #      Thu Mar 24 16:07:44 2016 -0700
> >> # Node ID ede9dd4902ae4a5cd464811532acdcd2fd02728f
> >> # Parent  0dd592daa18caead5b26cfc1f438b5d885d8b8ce
> >> convert: keep converted parents that are outside convert.hg.revs (BC)
> >>
> >> Before, when converting revisions without also converting their parents,
> >> parents that already had been converted 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 has been converted.  
> > I think the direction is good.
> >  
> >> --- a/hgext/convert/filemap.py
> >> +++ b/hgext/convert/filemap.py
> >> @@ -259,7 +259,7 @@ class filemap_source(common.converter_so
> >>       def getheads(self):
> >>           return self.base.getheads()
> >>   
> >> -    def getcommit(self, rev):
> >> +    def getcommit(self, rev, revmap=None):
> >>           # We want to save a reference to the commit objects to be able
> >>           # to rewrite their parents later on.
> >>           c = self.commits[rev] = self.base.getcommit(rev)  
> > Perhaps filemap_source should forward revmap to base source, but I got
> > IndexError if I changed it in that way.
> >
> > Can we pass a copy of the known revmap by constructor? I think it will be
> > simpler because revmap isn't necessary to be updated dynamically.  
> 
> I don't see a good way to do that with the current convert architecture. 
> The source and destination are created independently before both are 
> passed to convert().

I don't have a good idea. It should be doable, but it doesn't look nice.

  destc = convertsink(...)
  revmap = mapfile(ui, destc.revmapfile())
  srcc = convertsource(..., dict(revmap))
  converter(ui, srcc, destc, revmap, ...)  # transfer ownership of revmap file

> The same result could also be achieved with other more or less intrusive 
> changes to how parents generally are handled or to the conversion 
> framework in general. I am not sure which one would be better.

That sounds better as the convertsource won't need a revmap trick. But I
don't know how much hard work would be necessary to achieve that.

Patch

diff --git a/hgext/convert/bzr.py b/hgext/convert/bzr.py
--- a/hgext/convert/bzr.py
+++ b/hgext/convert/bzr.py
@@ -155,7 +155,7 @@  class bzr_source(common.converter_source
         files, changes = self._gettreechanges(self._revtree, prevtree)
         return files, changes, set()
 
-    def getcommit(self, version):
+    def getcommit(self, version, revmap=None):
         rev = self.sourcerepo.get_revision(version)
         # populate parent id cache
         if not rev.parent_ids:
diff --git a/hgext/convert/common.py b/hgext/convert/common.py
--- a/hgext/convert/common.py
+++ b/hgext/convert/common.py
@@ -132,7 +132,7 @@  class converter_source(object):
         """
         raise NotImplementedError
 
-    def getcommit(self, version):
+    def getcommit(self, version, revmap=None):
         """Return the commit object for version"""
         raise NotImplementedError
 
diff --git a/hgext/convert/convcmd.py b/hgext/convert/convcmd.py
--- a/hgext/convert/convcmd.py
+++ b/hgext/convert/convcmd.py
@@ -439,7 +439,7 @@  class converter(object):
         afile.close()
 
     def cachecommit(self, rev):
-        commit = self.source.getcommit(rev)
+        commit = self.source.getcommit(rev, revmap=self.map)
         commit.author = self.authors.get(commit.author, commit.author)
         commit.branch = mapbranch(commit.branch, self.branchmap)
         self.commitcache[rev] = commit
diff --git a/hgext/convert/cvs.py b/hgext/convert/cvs.py
--- a/hgext/convert/cvs.py
+++ b/hgext/convert/cvs.py
@@ -284,7 +284,7 @@  class convert_cvs(converter_source):
         self._parse()
         return sorted(self.files[rev].iteritems()), {}, set()
 
-    def getcommit(self, rev):
+    def getcommit(self, rev, revmap=None):
         self._parse()
         return self.changeset[rev]
 
diff --git a/hgext/convert/darcs.py b/hgext/convert/darcs.py
--- a/hgext/convert/darcs.py
+++ b/hgext/convert/darcs.py
@@ -146,7 +146,7 @@  class darcs_source(common.converter_sour
     def getheads(self):
         return self.parents[None]
 
-    def getcommit(self, rev):
+    def getcommit(self, rev, revmap=None):
         elt = self.changes[rev]
         date = util.strdate(elt.get('local_date'), '%a %b %d %H:%M:%S %Z %Y')
         desc = elt.findtext('name') + '\n' + elt.findtext('comment', '')
diff --git a/hgext/convert/filemap.py b/hgext/convert/filemap.py
--- a/hgext/convert/filemap.py
+++ b/hgext/convert/filemap.py
@@ -259,7 +259,7 @@  class filemap_source(common.converter_so
     def getheads(self):
         return self.base.getheads()
 
-    def getcommit(self, rev):
+    def getcommit(self, rev, revmap=None):
         # We want to save a reference to the commit objects to be able
         # to rewrite their parents later on.
         c = self.commits[rev] = self.base.getcommit(rev)
diff --git a/hgext/convert/git.py b/hgext/convert/git.py
--- a/hgext/convert/git.py
+++ b/hgext/convert/git.py
@@ -301,7 +301,7 @@  class convert_git(common.converter_sourc
                 changes.append(('.hgsubstate', ''))
         return (changes, copies, set())
 
-    def getcommit(self, version):
+    def getcommit(self, version, revmap=None):
         c = self.catfile(version, "commit") # read the commit hash
         end = c.find("\n\n")
         message = c[end + 2:]
diff --git a/hgext/convert/gnuarch.py b/hgext/convert/gnuarch.py
--- a/hgext/convert/gnuarch.py
+++ b/hgext/convert/gnuarch.py
@@ -181,7 +181,7 @@  class gnuarch_source(common.converter_so
         self.lastrev = rev
         return sorted(set(changes)), copies, set()
 
-    def getcommit(self, rev):
+    def getcommit(self, rev, revmap=None):
         changes = self.changes[rev]
         return common.commit(author=changes.author, date=changes.date,
                              desc=changes.summary, parents=self.parents[rev],
diff --git a/hgext/convert/hg.py b/hgext/convert/hg.py
--- a/hgext/convert/hg.py
+++ b/hgext/convert/hg.py
@@ -501,8 +501,10 @@  class mercurial_source(common.converter_
             self.lastrev = rev
         return self.lastctx
 
-    def _parents(self, ctx):
-        return [p for p in ctx.parents() if p and self.keep(p.node())]
+    def _parents(self, ctx, revmap=None):
+        return [p for p in ctx.parents()
+                if p and (self.keep(p.node()) or
+                          revmap is not None and nodemod.hex(p.node()) in revmap)]
 
     def getheads(self):
         return [nodemod.hex(h) for h in self._heads if self.keep(h)]
@@ -579,9 +581,9 @@  class mercurial_source(common.converter_
                 self.ui.warn(_('ignoring: %s\n') % e)
         return copies
 
-    def getcommit(self, rev):
+    def getcommit(self, rev, revmap=None):
         ctx = self._changectx(rev)
-        parents = [p.hex() for p in self._parents(ctx)]
+        parents = [p.hex() for p in self._parents(ctx, revmap)]
         crev = rev
 
         return common.commit(author=ctx.user(),
diff --git a/hgext/convert/monotone.py b/hgext/convert/monotone.py
--- a/hgext/convert/monotone.py
+++ b/hgext/convert/monotone.py
@@ -303,7 +303,7 @@  class monotone_source(common.converter_s
         node, attr = self.files.get(name, (None, ""))
         return data, attr
 
-    def getcommit(self, rev):
+    def getcommit(self, rev, revmap=None):
         extra = {}
         certs = self.mtngetcerts(rev)
         if certs.get('suspend') == certs["branch"]:
diff --git a/hgext/convert/p4.py b/hgext/convert/p4.py
--- a/hgext/convert/p4.py
+++ b/hgext/convert/p4.py
@@ -286,7 +286,7 @@  class p4_source(common.converter_source)
             raise error.Abort(_("convert from p4 does not support --full"))
         return self.files[rev], self.copies[rev], set()
 
-    def getcommit(self, rev):
+    def getcommit(self, rev, revmap=None):
         return self.changeset[rev]
 
     def gettags(self):
diff --git a/hgext/convert/subversion.py b/hgext/convert/subversion.py
--- a/hgext/convert/subversion.py
+++ b/hgext/convert/subversion.py
@@ -507,7 +507,7 @@  class svn_source(converter_source):
         self._changescache = (rev, (files, copies))
         return [f[0] for f in files]
 
-    def getcommit(self, rev):
+    def getcommit(self, rev, revmap=None):
         if rev not in self.commits:
             uuid, module, revnum = revsplit(rev)
             self.module = module
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