Patchwork D6643: convert: add a config option to help doing identity hg->hg conversion

login
register
mail settings
Submitter phabricator
Date July 14, 2019, 7:17 p.m.
Message ID <differential-rev-PHID-DREV-vbgemsq2occ5tc5s43oz-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/40922/
State Superseded
Headers show

Comments

phabricator - July 14, 2019, 7:17 p.m.
valentin.gatienbaron created this revision.
Herald added subscribers: mercurial-devel, mjpieters.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  I want to change the computation of the list of files modified by a
  commit. In principle, this would simply change a cache. But since this
  information is stored in commits rather than a cache, changing it
  means changing commit hashes (going forward).
  
  Some users rely on the convert extension from hg to hg not changing
  hashes when nothing changes (usually). Allow these users to preserve
  hashes despite changes to the changelog files computation by reusing
  these files lists when the manifest is unchanged (since these files
  list are derived from the manifest).

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6643

AFFECTED FILES
  hgext/convert/__init__.py
  hgext/convert/common.py
  hgext/convert/hg.py
  hgext/lfs/__init__.py
  mercurial/configitems.py
  mercurial/localrepo.py
  tests/test-convert-identity.t
  tests/test-convert.t

CHANGE DETAILS




To: valentin.gatienbaron, #hg-reviewers
Cc: mjpieters, mercurial-devel
phabricator - July 15, 2019, 4:20 p.m.
This revision now requires changes to proceed.
mharbison72 added a comment.
mharbison72 requested changes to this revision.


  Don't take this as a complete list, but it looks like repository.py, remotefilelog, eol, and probably keyword need to be taught the new kwarg.  Note that the keyword wrapper is called `kwcommitctx`, so it isn't enough to grep for `def commitctx`.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6643/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6643

To: valentin.gatienbaron, #hg-reviewers, mharbison72
Cc: mharbison72, mjpieters, mercurial-devel
phabricator - July 15, 2019, 10:11 p.m.
valentin.gatienbaron added a comment.


  In D6643#97171 <https://phab.mercurial-scm.org/D6643#97171>, @mharbison72 wrote:
  
  > Don't take this as a complete list, but it looks like repository.py, remotefilelog, eol, and probably keyword need to be taught the new kwarg.  Note that the keyword wrapper is called `kwcommitctx`, so it isn't enough to grep for `def commitctx`.
  
  I found the same places by grepping for commitctx, plus a minor one in a test, which I fixed.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6643/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6643

To: valentin.gatienbaron, #hg-reviewers, mharbison72
Cc: mharbison72, mjpieters, mercurial-devel
phabricator - July 17, 2019, 1:33 a.m.
mharbison72 added a comment.
mharbison72 accepted this revision.


  The update worked for me, thanks.
  
  I ran this on the repo that was giving me fits last summer, and it stayed consistent up until just after the point where there was an octopus fixup merge.  I suspect the logic in that part of convert is too loose (i.e. it should be dropping files in the fixup merge that were already in the first merge if there weren't changes in the 3rd branch), but wasn't able to write a simple test.  (The original repo was in bzr, converted to hg, and then I had to reconvert to lfs.)  I've been swamped at work, so I don't have time to dig into it.  But no sense in holding this up, as there doesn't seem to be a regression here.
  
  Do you think there are other hash preserving things that can be added?  (Not asking you to do that.)  Or should we save this config name for such functionality?  I don't have any better name ideas, and don't have a problem adding more to this config option.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6643/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6643

To: valentin.gatienbaron, #hg-reviewers, mharbison72
Cc: mharbison72, mjpieters, mercurial-devel
phabricator - July 17, 2019, 2:50 a.m.
valentin.gatienbaron added a comment.


  In D6643#97247 <https://phab.mercurial-scm.org/D6643#97247>, @mharbison72 wrote:
  
  > The update worked for me, thanks.
  
  Cool, thanks.
  
  > I ran this on the repo that was giving me fits last summer, and it stayed consistent up until just after the point where there was an octopus fixup merge.  I suspect the logic in that part of convert is too loose (i.e. it should be dropping files in the fixup merge that were already in the first merge if there weren't changes in the 3rd branch), but wasn't able to write a simple test.  (The original repo was in bzr, converted to hg, and then I had to reconvert to lfs.)  I've been swamped at work, so I don't have time to dig into it.  But no sense in holding this up, as there doesn't seem to be a regression here.
  
  I didn't quite understand. Are you saying your convert call cause hashes to change after this octopus merge, but it's not caused by my change, i.e. hg behaves this way even before?
  
  > Do you think there are other hash preserving things that can be added?  (Not asking you to do that.)  Or should we save this config name for such functionality?  I don't have any better name ideas, and don't have a problem adding more to this config option.
  
  I don't plan on making more changes that impact hashes at least (other than more perhaps tweaking the changelog files list some more).
  Changes to the choice of p1/p2 in manifestlog/filelogs revisions would cause spurious hash changes. Changes in this area don't seem that likely though.
  But if such a change did happen, you'd probably want to preserve hashes regardless of the specific source of the hash change. And if not, more specialized options can be minted.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6643/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6643

To: valentin.gatienbaron, #hg-reviewers, mharbison72
Cc: mharbison72, mjpieters, mercurial-devel
phabricator - July 17, 2019, 4:17 a.m.
mharbison72 added a comment.


  In D6643#97250 <https://phab.mercurial-scm.org/D6643#97250>, @valentin.gatienbaron wrote:
  
  > In D6643#97247 <https://phab.mercurial-scm.org/D6643#97247>, @mharbison72 wrote:
  >
  >> I ran this on the repo that was giving me fits last summer, and it stayed consistent up until just after the point where there was an octopus fixup merge.  I suspect the logic in that part of convert is too loose (i.e. it should be dropping files in the fixup merge that were already in the first merge if there weren't changes in the 3rd branch), but wasn't able to write a simple test.  (The original repo was in bzr, converted to hg, and then I had to reconvert to lfs.)  I've been swamped at work, so I don't have time to dig into it.  But no sense in holding this up, as there doesn't seem to be a regression here.
  >
  > I didn't quite understand. Are you saying your convert call cause hashes to change after this octopus merge, but it's not caused by my change, i.e. hg behaves this way even before?
  
  Yes, it was a pre-existing issue.  It's the problem that I referenced here, which your patch got to without any of the hacking that I did:
  
    https://www.mercurial-scm.org/pipermail/mercurial-devel/2018-July/118896.html
  
  >> Do you think there are other hash preserving things that can be added?  (Not asking you to do that.)  Or should we save this config name for such functionality?  I don't have any better name ideas, and don't have a problem adding more to this config option.
  >
  > I don't plan on making more changes that impact hashes at least (other than more perhaps tweaking the changelog files list some more).
  > Changes to the choice of p1/p2 in manifestlog/filelogs revisions would cause spurious hash changes. Changes in this area don't seem that likely though.
  > But if such a change did happen, you'd probably want to preserve hashes regardless of the specific source of the hash change. And if not, more specialized options can be minted.
  
  Correct.  I just wanted to make sure that nobody has a problem with this config morphing into a general "try really hard to keep hashes" setting in the future, if other tweaks need to be made.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6643/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6643

To: valentin.gatienbaron, #hg-reviewers, mharbison72
Cc: mharbison72, mjpieters, mercurial-devel

Patch

diff --git a/tests/test-convert.t b/tests/test-convert.t
--- a/tests/test-convert.t
+++ b/tests/test-convert.t
@@ -373,6 +373,11 @@ 
                     records the given string as a 'convert_source' extra value
                     on each commit made in the target repository. The default is
                     None.
+      convert.hg.preserve-hash
+                    only works with mercurial sources. Make convert prevent
+                    performance improvement to the list of modified files in
+                    commits when such an improvement would cause the hash of a
+                    commit to change. The default is False.
   
       All Destinations
       ################
diff --git a/tests/test-convert-identity.t b/tests/test-convert-identity.t
new file mode 100644
--- /dev/null
+++ b/tests/test-convert-identity.t
@@ -0,0 +1,40 @@ 
+Testing that convert.hg.preserve-hash=true can be used to make hg
+convert from hg repo to hg repo preserve hashes, even if the
+computation of the files list in commits change slightly between hg
+versions.
+
+  $ cat <<'EOF' >> "$HGRCPATH"
+  > [extensions]
+  > convert =
+  > EOF
+  $ cat <<'EOF' > changefileslist.py
+  > from mercurial import (changelog, extensions)
+  > def wrap(orig, clog, manifest, files, *args, **kwargs):
+  >   return orig(clog, manifest, ["a"], *args, **kwargs)
+  > def extsetup(ui):
+  >   extensions.wrapfunction(changelog.changelog, 'add', wrap)
+  > EOF
+
+  $ hg init repo
+  $ cd repo
+  $ echo a > a; hg commit -qAm a
+  $ echo b > a; hg commit -qAm b
+  $ hg up -qr 0; echo c > c; hg commit -qAm c
+  $ hg merge -qr 1
+  $ hg commit -m_ --config extensions.x=../changefileslist.py
+  $ hg log -r . -T '{node|short} {files|json}\n'
+  c085bbe93d59 ["a"]
+
+Now that we have a commit with a files list that's not what the
+current hg version would create, check that convert either fixes it or
+keeps it depending on config:
+
+  $ hg convert -q . ../convert
+  $ hg --cwd ../convert log -r tip -T '{node|short} {files|json}\n'
+  b7c4d4bbacd3 []
+  $ rm -rf ../convert
+
+  $ hg convert -q . ../convert --config convert.hg.preserve-hash=true
+  $ hg --cwd ../convert log -r tip -T '{node|short} {files|json}\n'
+  c085bbe93d59 ["a"]
+  $ rm -rf ../convert
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -2578,7 +2578,7 @@ 
         return ret
 
     @unfilteredmethod
-    def commitctx(self, ctx, error=False):
+    def commitctx(self, ctx, error=False, origctx=None):
         """Add a new revision to current repository.
         Revision information is passed via the context argument.
 
@@ -2586,6 +2586,12 @@ 
         modified/added/removed files. On merge, it may be wider than the
         ctx.files() to be committed, since any file nodes derived directly
         from p1 or p2 are excluded from the committed ctx.files().
+
+        origctx is for convert to work around the problem that bug
+        fixes to the files list in changesets change hashes. For
+        convert to be the identity, it can pass an origctx and this
+        function will use the same files list when it makes sense to
+        do so.
         """
 
         p1, p2 = ctx.p1(), ctx.p2()
@@ -2701,6 +2707,9 @@ 
                 filesadded = filesadded or None
                 filesremoved = filesremoved or None
 
+            if origctx and origctx.manifestnode() == mn:
+                files = origctx.files()
+
             # update changelog
             self.ui.note(_("committing changelog\n"))
             self.changelog.delayupdate(tr)
diff --git a/mercurial/configitems.py b/mercurial/configitems.py
--- a/mercurial/configitems.py
+++ b/mercurial/configitems.py
@@ -291,6 +291,9 @@ 
 coreconfigitem('convert', 'hg.ignoreerrors',
     default=False,
 )
+coreconfigitem('convert', 'hg.preserve-hash',
+    default=False,
+)
 coreconfigitem('convert', 'hg.revs',
     default=None,
 )
diff --git a/hgext/lfs/__init__.py b/hgext/lfs/__init__.py
--- a/hgext/lfs/__init__.py
+++ b/hgext/lfs/__init__.py
@@ -227,9 +227,9 @@ 
 
     class lfsrepo(repo.__class__):
         @localrepo.unfilteredmethod
-        def commitctx(self, ctx, error=False):
+        def commitctx(self, ctx, error=False, origctx=None):
             repo.svfs.options['lfstrack'] = _trackedmatcher(self)
-            return super(lfsrepo, self).commitctx(ctx, error)
+            return super(lfsrepo, self).commitctx(ctx, error, origctx=origctx)
 
     repo.__class__ = lfsrepo
 
diff --git a/hgext/convert/hg.py b/hgext/convert/hg.py
--- a/hgext/convert/hg.py
+++ b/hgext/convert/hg.py
@@ -339,7 +339,11 @@ 
                                    phases.phasenames[commit.phase], 'convert')
 
             with self.repo.transaction("convert") as tr:
-                node = nodemod.hex(self.repo.commitctx(ctx))
+                if self.repo.ui.config('convert', 'hg.preserve-hash'):
+                    origctx = commit.ctx
+                else:
+                    origctx = None
+                node = nodemod.hex(self.repo.commitctx(ctx, origctx=origctx))
 
                 # If the node value has changed, but the phase is lower than
                 # draft, set it back to draft since it hasn't been exposed
@@ -591,7 +595,8 @@ 
                              extra=ctx.extra(),
                              sortkey=ctx.rev(),
                              saverev=self.saverev,
-                             phase=ctx.phase())
+                             phase=ctx.phase(),
+                             ctx=ctx)
 
     def numcommits(self):
         return len(self.repo)
diff --git a/hgext/convert/common.py b/hgext/convert/common.py
--- a/hgext/convert/common.py
+++ b/hgext/convert/common.py
@@ -114,7 +114,7 @@ 
 class commit(object):
     def __init__(self, author, date, desc, parents, branch=None, rev=None,
                  extra=None, sortkey=None, saverev=True, phase=phases.draft,
-                 optparents=None):
+                 optparents=None, ctx=None):
         self.author = author or 'unknown'
         self.date = date or '0 0'
         self.desc = desc
@@ -126,6 +126,7 @@ 
         self.sortkey = sortkey
         self.saverev = saverev
         self.phase = phase
+        self.ctx = ctx # for hg to hg conversions
 
 class converter_source(object):
     """Conversion source interface"""
diff --git a/hgext/convert/__init__.py b/hgext/convert/__init__.py
--- a/hgext/convert/__init__.py
+++ b/hgext/convert/__init__.py
@@ -439,6 +439,11 @@ 
     :convert.hg.sourcename: records the given string as a 'convert_source' extra
         value on each commit made in the target repository. The default is None.
 
+    :convert.hg.preserve-hash: only works with mercurial sources. Make convert
+        prevent performance improvement to the list of modified files in commits
+        when such an improvement would cause the hash of a commit to change.
+        The default is False.
+
     All Destinations
     ################