Patchwork [evolve-ext] evolve refactor

login
register
mail settings
Submitter Shusen LIU
Date Dec. 2, 2015, 6:03 p.m.
Message ID <5a3e42a1f87b0678acc2.1449079419@dev1221.lla1.facebook.com>
Download mbox | patch
Permalink /patch/11764/
State Changes Requested
Delegated to: Pierre-Yves David
Headers show

Comments

Shusen LIU - Dec. 2, 2015, 6:03 p.m.
# HG changeset patch
# User LIU Shusen
# Date 1446223332 25200
#      Fri Oct 30 09:42:12 2015 -0700
# Node ID 5a3e42a1f87b0678acc2c3cdc952614c09c23bcf
# Parent  c4f8a2916e43cd415113436da78e5f7c6cc76ff1
evolve refactor
Laurent Charignon - Dec. 3, 2015, 12:37 a.m.
Hi Shusen,

Thanks for sending this patch to the list.
To make review easier, we like to separate code in smaller patches with one minimal change per patch like "extracting code into a function".
This patch needs to be sliced in smaller patches. 
I would do things in that order for example, but feel free to pick a different order:
- _evolvecommit (How about _relocatecommit for the name instead?)
- _evolvecleanup (I am not a fan of the name as it does not really perform the cleanup for the evolve command and there is already a cleanup method, how about _finalizerelocate?)
- _evolvestatedelete
- _evolvestateread 
- _evolvestatewrite 
- _evolvemerge
- _evolvecontinue and the final bit of logic
- Add a test for bumped with conflict (I believe that it wouldn't work with the code as is as there is still a reference to the graft state)


> On Dec 2, 2015, at 10:03 AM, Shusen LIU <liushusen@fb.com> wrote:
> 
> # HG changeset patch
> # User LIU Shusen
> # Date 1446223332 25200
> #      Fri Oct 30 09:42:12 2015 -0700
> # Node ID 5a3e42a1f87b0678acc2c3cdc952614c09c23bcf
> # Parent  c4f8a2916e43cd415113436da78e5f7c6cc76ff1
> evolve refactor
> 
> diff --git a/hgext/evolve.py b/hgext/evolve.py
> --- a/hgext/evolve.py
> +++ b/hgext/evolve.py
> @@ -111,6 +111,8 @@
> from mercurial import wireproto
> from mercurial import localrepo
> from mercurial.hgweb import hgweb_mod
> +from mercurial.lock import release
> +
> 
> cmdtable = {}
> command = cmdutil.command(cmdtable)
> @@ -899,9 +901,7 @@
>         raise util.Abort(
>             'no support for evolving merge changesets yet',
>             hint="Redo the merge and use `hg prune <old> --succ <new>` to obsolete the old one")
> -    destbookmarks = repo.nodebookmarks(dest.node())
> -    nodesrc = orig.node()
> -    destphase = repo[nodesrc].phase()
> +
>     commitmsg = orig.description()
> 
>     cache = {}
> @@ -931,33 +931,17 @@
>     tr = repo.transaction('relocate')
>     try:
>         try:
> -            if repo['.'].rev() != dest.rev():
> -                merge.update(repo, dest, False, True, False)
> -            if bmactive(repo):
> -                repo.ui.status(_("(leaving bookmark %s)\n") % bmactive(repo))
> -            bmdeactivate(repo)
> -            if keepbranch:
> -                repo.dirstate.setbranch(orig.branch())
> -            r = merge.graft(repo, orig, orig.p1(), ['local', 'graft'])
> +            r = _evolvemerge(repo, orig, dest, keepbranch)
> +
>             if r[-1]:  #some conflict
> +                _evolvestatewrite(repo, {'orig': orig.hex(),
> +                                         'dest': dest.hex(),
> +                                         'commitmsg': commitmsg,
> +                                        })
>                 raise util.Abort(
>                         'unresolved merge conflicts (see hg help resolve)')
> -            if commitmsg is None:
> -                commitmsg = orig.description()
> -            extra = dict(orig.extra())
> -            if 'branch' in extra:
> -                del extra['branch']
> -            extra['rebase_source'] = orig.hex()
> -
> -            backup = repo.ui.backupconfig('phases', 'new-commit')
> -            try:
> -                targetphase = max(orig.phase(), phases.draft)
> -                repo.ui.setconfig('phases', 'new-commit', targetphase, 'rebase')
> -                # Commit might fail if unresolved files exist
> -                nodenew = repo.commit(text=commitmsg, user=orig.user(),
> -                                      date=orig.date(), extra=extra)
> -            finally:
> -                repo.ui.restoreconfig(backup)
> +
> +            nodenew = _evolvecommit(repo, orig, commitmsg)
>         except util.Abort, exc:
>             repo.dirstate.beginparentchange()
>             repo.setparents(repo['.'].node(), nullid)
> @@ -969,26 +953,113 @@
>                 pass
>             exc.__class__ = LocalMergeFailure
>             raise
> -        oldbookmarks = repo.nodebookmarks(nodesrc)
> -        if nodenew is not None:
> -            phases.retractboundary(repo, tr, destphase, [nodenew])
> -            obsolete.createmarkers(repo, [(repo[nodesrc], (repo[nodenew],))])
> -            for book in oldbookmarks:
> -                repo._bookmarks[book] = nodenew
> -        else:
> -            obsolete.createmarkers(repo, [(repo[nodesrc], ())])
> -            # Behave like rebase, move bookmarks to dest
> -            for book in oldbookmarks:
> -                repo._bookmarks[book] = dest.node()
> -        for book in destbookmarks: # restore bookmark that rebase move
> -            repo._bookmarks[book] = dest.node()
> -        if oldbookmarks or destbookmarks:
> -            repo._bookmarks.recordchange(tr)
> +
> +        _evolveclean(repo, orig, dest, nodenew, tr)
> +        _evolvestatedelete(repo)
>         tr.close()
>     finally:
>         tr.release()
>     return nodenew
> 
> +def _evolvestatewrite(repo, data):
> +    repo.vfs.write('evolvestate',
> +                    '|'.join([data['orig'], data['dest'], data['commitmsg']]))

How does this work with bumped commits?
This will fail with commit messages containing pipe characters, how about NULL bytes to separate the fields?

> +
> +def _evolvestateread(repo):
> +    orig, dest, commitmsg = repo.vfs.read('evolvestate').split('|')
> +    return { 'orig': orig,
> +             'dest': dest,
> +             'commitmsg': commitmsg, }
> +
> +def _evolvestatedelete(repo):
> +    util.unlinkpath(repo.join('evolvestate'), ignoremissing=True)
> +
> +def _evolvecontinue(repo):
> +    state = _evolvestateread(repo)
> +    orig = repo[state['orig']]
> +    dest = repo[state['dest']]
> +    commitmsg = state['commitmsg']
> +
> +    desc = '%d:%s "%s"' % (orig.rev(), orig,
> +                           orig.description().split('\n', 1)[0])
> +    names = repo.nodetags(orig.node()) + repo.nodebookmarks(orig.node())
> +    if names:
> +        desc += ' (%s)' % ' '.join(names)
> +    repo.ui.status(_('evolving %s\n') % desc)
> +
> +    wlock = lock = None
> +    try:
> +        wlock = repo.wlock() # must come first
> +        lock = repo.lock()
> +        tr = repo.transaction("evolvecontinue")
> +        # perform write operation
> +        nodenew = _evolvecommit(repo, orig, commitmsg, True)
> +        _evolveclean(repo, orig, dest, nodenew, tr)
> +        _evolvestatedelete(repo)
> +        tr.close()
> +    finally:
> +        release(tr, lock, wlock) # reverse order
> +
> +def _evolvemerge(repo, orig, dest, keepbranch):
> +    if repo['.'].rev() != dest.rev():
> +        merge.update(repo, dest, False, True, False)
> +    if bmactive(repo):
> +        repo.ui.status(_("(leaving bookmark %s)\n") % bmactive(repo))
> +    bmdeactivate(repo)
> +    if keepbranch:
> +        repo.dirstate.setbranch(orig.branch())
> +    r = merge.graft(repo, orig, orig.p1(), ['local', 'graft'])
> +    return r
> +
> +def _evolvecommit(repo, orig, commitmsg, continued=False):
> +    if commitmsg is None:
> +        commitmsg = orig.description()
> +    extra = dict(orig.extra())
> +    if 'branch' in extra:
> +        del extra['branch']
> +
> +    if continued:
> +        source = orig.extra().get('source')
> +        if source:
> +            extra['source'] = source
> +            extra['intermediate-source'] = orig.hex()
> +        else:
> +            extra['source'] = orig.hex()
> +        extra.pop('rebase_source', None)
> +    else:
> +        extra['rebase_source'] = orig.hex()
> +
> +    backup = repo.ui.backupconfig('phases', 'new-commit')
> +    try:
> +        targetphase = max(orig.phase(), phases.draft)
> +        repo.ui.setconfig('phases', 'new-commit', targetphase, 'rebase')
> +        # Commit might fail if unresolved files exist
> +        nodenew = repo.commit(text=commitmsg, user=orig.user(),
> +                              date=orig.date(), extra=extra)
> +    finally:
> +        repo.ui.restoreconfig(backup)
> +    return nodenew
> +
> +def _evolveclean(repo, orig, dest, nodenew, tr):
> +    destbookmarks = repo.nodebookmarks(dest.node())
> +    nodesrc = orig.node()
> +    destphase = repo[nodesrc].phase()
> +    oldbookmarks = repo.nodebookmarks(nodesrc)
> +    if nodenew is not None:
> +        phases.retractboundary(repo, tr, destphase, [nodenew])
> +        obsolete.createmarkers(repo, [(repo[nodesrc], (repo[nodenew],))])
> +        for book in oldbookmarks:
> +            repo._bookmarks[book] = nodenew
> +    else:
> +        obsolete.createmarkers(repo, [(repo[nodesrc], ())])
> +        # Behave like rebase, move bookmarks to dest
> +        for book in oldbookmarks:
> +            repo._bookmarks[book] = dest.node()
> +    for book in destbookmarks: # restore bookmark that rebase move
> +        repo._bookmarks[book] = dest.node()
> +    if oldbookmarks or destbookmarks:
> +        repo._bookmarks.recordchange(tr)
> +
> def _bookmarksupdater(repo, oldid, tr):
>     """Return a callable update(newid) updating the current bookmark
>     and bookmarks bound to oldid to newid.
> @@ -1627,8 +1698,10 @@
>             raise util.Abort('cannot specify both "--any" and "--continue"')
>         if allopt:
>             raise util.Abort('cannot specify both "--all" and "--continue"')
> -        graftcmd = commands.table['graft'][0]
> -        return graftcmd(ui, repo, old_obsolete=True, **{'continue': True})
> +        # graftcmd = commands.table['graft'][0]
> +        # return graftcmd(ui, repo, old_obsolete=True, **{'continue': True})
> +        _evolvecontinue(repo)
> +        return 0
>     cmdutil.bailifchanged(repo)
> 
> 
> @@ -1764,7 +1837,7 @@
>         try:
>             relocate(repo, orig, target, keepbranch)
>         except MergeFailure:
> -            repo.opener.write('graftstate', orig.hex() + '\n')
> +            # repo.opener.write('graftstate', orig.hex() + '\n')
>             repo.ui.write_err(_('evolve failed!\n'))
>             repo.ui.write_err(
>                 _('fix conflict and run "hg evolve --continue"'
> diff --git a/tests/test-stabilize-conflict.t b/tests/test-stabilize-conflict.t
> --- a/tests/test-stabilize-conflict.t
> +++ b/tests/test-stabilize-conflict.t
> @@ -166,7 +166,7 @@
>   $ hg resolve --all -m
>   (no more unresolved files)
>   $ hg evolve --continue
> -  grafting 5:71c18f70c34f "babar count up to fifteen"
> +  evolving 5:71c18f70c34f "babar count up to fifteen"
>   $ hg resolve -l
>   $ hg log -G
>   @  changeset:   8:1836b91c6c1d
> diff --git a/tests/test-stabilize-result.t b/tests/test-stabilize-result.t
> --- a/tests/test-stabilize-result.t
> +++ b/tests/test-stabilize-result.t
> @@ -97,13 +97,13 @@
>   +a
>   +newer a
>   $ hg evolve --continue
> -  grafting 5:3655f0f50885 "newer a"
> +  evolving 5:3655f0f50885 "newer a"
>   abort: unresolved merge conflicts (see "hg help resolve")
>   [255]
>   $ hg resolve -m a
>   (no more unresolved files)
>   $ hg evolve --continue
> -  grafting 5:3655f0f50885 "newer a"
> +  evolving 5:3655f0f50885 "newer a"
> 
> Stabilize latecomer with different parent
> =========================================
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel


Thanks again,

Laurent

Patch

diff --git a/hgext/evolve.py b/hgext/evolve.py
--- a/hgext/evolve.py
+++ b/hgext/evolve.py
@@ -111,6 +111,8 @@ 
 from mercurial import wireproto
 from mercurial import localrepo
 from mercurial.hgweb import hgweb_mod
+from mercurial.lock import release
+
 
 cmdtable = {}
 command = cmdutil.command(cmdtable)
@@ -899,9 +901,7 @@ 
         raise util.Abort(
             'no support for evolving merge changesets yet',
             hint="Redo the merge and use `hg prune <old> --succ <new>` to obsolete the old one")
-    destbookmarks = repo.nodebookmarks(dest.node())
-    nodesrc = orig.node()
-    destphase = repo[nodesrc].phase()
+
     commitmsg = orig.description()
 
     cache = {}
@@ -931,33 +931,17 @@ 
     tr = repo.transaction('relocate')
     try:
         try:
-            if repo['.'].rev() != dest.rev():
-                merge.update(repo, dest, False, True, False)
-            if bmactive(repo):
-                repo.ui.status(_("(leaving bookmark %s)\n") % bmactive(repo))
-            bmdeactivate(repo)
-            if keepbranch:
-                repo.dirstate.setbranch(orig.branch())
-            r = merge.graft(repo, orig, orig.p1(), ['local', 'graft'])
+            r = _evolvemerge(repo, orig, dest, keepbranch)
+
             if r[-1]:  #some conflict
+                _evolvestatewrite(repo, {'orig': orig.hex(),
+                                         'dest': dest.hex(),
+                                         'commitmsg': commitmsg,
+                                        })
                 raise util.Abort(
                         'unresolved merge conflicts (see hg help resolve)')
-            if commitmsg is None:
-                commitmsg = orig.description()
-            extra = dict(orig.extra())
-            if 'branch' in extra:
-                del extra['branch']
-            extra['rebase_source'] = orig.hex()
-
-            backup = repo.ui.backupconfig('phases', 'new-commit')
-            try:
-                targetphase = max(orig.phase(), phases.draft)
-                repo.ui.setconfig('phases', 'new-commit', targetphase, 'rebase')
-                # Commit might fail if unresolved files exist
-                nodenew = repo.commit(text=commitmsg, user=orig.user(),
-                                      date=orig.date(), extra=extra)
-            finally:
-                repo.ui.restoreconfig(backup)
+
+            nodenew = _evolvecommit(repo, orig, commitmsg)
         except util.Abort, exc:
             repo.dirstate.beginparentchange()
             repo.setparents(repo['.'].node(), nullid)
@@ -969,26 +953,113 @@ 
                 pass
             exc.__class__ = LocalMergeFailure
             raise
-        oldbookmarks = repo.nodebookmarks(nodesrc)
-        if nodenew is not None:
-            phases.retractboundary(repo, tr, destphase, [nodenew])
-            obsolete.createmarkers(repo, [(repo[nodesrc], (repo[nodenew],))])
-            for book in oldbookmarks:
-                repo._bookmarks[book] = nodenew
-        else:
-            obsolete.createmarkers(repo, [(repo[nodesrc], ())])
-            # Behave like rebase, move bookmarks to dest
-            for book in oldbookmarks:
-                repo._bookmarks[book] = dest.node()
-        for book in destbookmarks: # restore bookmark that rebase move
-            repo._bookmarks[book] = dest.node()
-        if oldbookmarks or destbookmarks:
-            repo._bookmarks.recordchange(tr)
+
+        _evolveclean(repo, orig, dest, nodenew, tr)
+        _evolvestatedelete(repo)
         tr.close()
     finally:
         tr.release()
     return nodenew
 
+def _evolvestatewrite(repo, data):
+    repo.vfs.write('evolvestate',
+                    '|'.join([data['orig'], data['dest'], data['commitmsg']]))
+
+def _evolvestateread(repo):
+    orig, dest, commitmsg = repo.vfs.read('evolvestate').split('|')
+    return { 'orig': orig,
+             'dest': dest,
+             'commitmsg': commitmsg, }
+
+def _evolvestatedelete(repo):
+    util.unlinkpath(repo.join('evolvestate'), ignoremissing=True)
+
+def _evolvecontinue(repo):
+    state = _evolvestateread(repo)
+    orig = repo[state['orig']]
+    dest = repo[state['dest']]
+    commitmsg = state['commitmsg']
+
+    desc = '%d:%s "%s"' % (orig.rev(), orig,
+                           orig.description().split('\n', 1)[0])
+    names = repo.nodetags(orig.node()) + repo.nodebookmarks(orig.node())
+    if names:
+        desc += ' (%s)' % ' '.join(names)
+    repo.ui.status(_('evolving %s\n') % desc)
+
+    wlock = lock = None
+    try:
+        wlock = repo.wlock() # must come first
+        lock = repo.lock()
+        tr = repo.transaction("evolvecontinue")
+        # perform write operation
+        nodenew = _evolvecommit(repo, orig, commitmsg, True)
+        _evolveclean(repo, orig, dest, nodenew, tr)
+        _evolvestatedelete(repo)
+        tr.close()
+    finally:
+        release(tr, lock, wlock) # reverse order
+
+def _evolvemerge(repo, orig, dest, keepbranch):
+    if repo['.'].rev() != dest.rev():
+        merge.update(repo, dest, False, True, False)
+    if bmactive(repo):
+        repo.ui.status(_("(leaving bookmark %s)\n") % bmactive(repo))
+    bmdeactivate(repo)
+    if keepbranch:
+        repo.dirstate.setbranch(orig.branch())
+    r = merge.graft(repo, orig, orig.p1(), ['local', 'graft'])
+    return r
+
+def _evolvecommit(repo, orig, commitmsg, continued=False):
+    if commitmsg is None:
+        commitmsg = orig.description()
+    extra = dict(orig.extra())
+    if 'branch' in extra:
+        del extra['branch']
+
+    if continued:
+        source = orig.extra().get('source')
+        if source:
+            extra['source'] = source
+            extra['intermediate-source'] = orig.hex()
+        else:
+            extra['source'] = orig.hex()
+        extra.pop('rebase_source', None)
+    else:
+        extra['rebase_source'] = orig.hex()
+
+    backup = repo.ui.backupconfig('phases', 'new-commit')
+    try:
+        targetphase = max(orig.phase(), phases.draft)
+        repo.ui.setconfig('phases', 'new-commit', targetphase, 'rebase')
+        # Commit might fail if unresolved files exist
+        nodenew = repo.commit(text=commitmsg, user=orig.user(),
+                              date=orig.date(), extra=extra)
+    finally:
+        repo.ui.restoreconfig(backup)
+    return nodenew
+
+def _evolveclean(repo, orig, dest, nodenew, tr):
+    destbookmarks = repo.nodebookmarks(dest.node())
+    nodesrc = orig.node()
+    destphase = repo[nodesrc].phase()
+    oldbookmarks = repo.nodebookmarks(nodesrc)
+    if nodenew is not None:
+        phases.retractboundary(repo, tr, destphase, [nodenew])
+        obsolete.createmarkers(repo, [(repo[nodesrc], (repo[nodenew],))])
+        for book in oldbookmarks:
+            repo._bookmarks[book] = nodenew
+    else:
+        obsolete.createmarkers(repo, [(repo[nodesrc], ())])
+        # Behave like rebase, move bookmarks to dest
+        for book in oldbookmarks:
+            repo._bookmarks[book] = dest.node()
+    for book in destbookmarks: # restore bookmark that rebase move
+        repo._bookmarks[book] = dest.node()
+    if oldbookmarks or destbookmarks:
+        repo._bookmarks.recordchange(tr)
+
 def _bookmarksupdater(repo, oldid, tr):
     """Return a callable update(newid) updating the current bookmark
     and bookmarks bound to oldid to newid.
@@ -1627,8 +1698,10 @@ 
             raise util.Abort('cannot specify both "--any" and "--continue"')
         if allopt:
             raise util.Abort('cannot specify both "--all" and "--continue"')
-        graftcmd = commands.table['graft'][0]
-        return graftcmd(ui, repo, old_obsolete=True, **{'continue': True})
+        # graftcmd = commands.table['graft'][0]
+        # return graftcmd(ui, repo, old_obsolete=True, **{'continue': True})
+        _evolvecontinue(repo)
+        return 0
     cmdutil.bailifchanged(repo)
 
 
@@ -1764,7 +1837,7 @@ 
         try:
             relocate(repo, orig, target, keepbranch)
         except MergeFailure:
-            repo.opener.write('graftstate', orig.hex() + '\n')
+            # repo.opener.write('graftstate', orig.hex() + '\n')
             repo.ui.write_err(_('evolve failed!\n'))
             repo.ui.write_err(
                 _('fix conflict and run "hg evolve --continue"'
diff --git a/tests/test-stabilize-conflict.t b/tests/test-stabilize-conflict.t
--- a/tests/test-stabilize-conflict.t
+++ b/tests/test-stabilize-conflict.t
@@ -166,7 +166,7 @@ 
   $ hg resolve --all -m
   (no more unresolved files)
   $ hg evolve --continue
-  grafting 5:71c18f70c34f "babar count up to fifteen"
+  evolving 5:71c18f70c34f "babar count up to fifteen"
   $ hg resolve -l
   $ hg log -G
   @  changeset:   8:1836b91c6c1d
diff --git a/tests/test-stabilize-result.t b/tests/test-stabilize-result.t
--- a/tests/test-stabilize-result.t
+++ b/tests/test-stabilize-result.t
@@ -97,13 +97,13 @@ 
   +a
   +newer a
   $ hg evolve --continue
-  grafting 5:3655f0f50885 "newer a"
+  evolving 5:3655f0f50885 "newer a"
   abort: unresolved merge conflicts (see "hg help resolve")
   [255]
   $ hg resolve -m a
   (no more unresolved files)
   $ hg evolve --continue
-  grafting 5:3655f0f50885 "newer a"
+  evolving 5:3655f0f50885 "newer a"
 
 Stabilize latecomer with different parent
 =========================================