[PATCH evolve-ext] evolve refactor

Laurent Charignon lcharignon at fb.com
Wed Dec 2 18:37:55 CST 2015


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 at 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 at selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel


Thanks again,

Laurent



More information about the Mercurial-devel mailing list