[PATCH 2 of 4 evolve-ext] metaedit: extend the functionality to support editing multiple commits

Ryan McElroy rm at fb.com
Mon Dec 5 08:14:58 EST 2016



On 11/16/16 7:56 PM, Mateusz Kwapich wrote:
> # HG changeset patch
> # User Mateusz Kwapich <mitrandir at fb.com>
> # Date 1479324135 0
> #      Wed Nov 16 19:22:15 2016 +0000
> # Branch stable
> # Node ID b2bde478bfebc390dba8f1ee314b7bdd062ab191
> # Parent  744c6acd84edf73ffdf505b9673b0383db727a0e
> metaedit: extend the functionality to support editing multiple commits
>
> diff --git a/hgext/evolve.py b/hgext/evolve.py
> --- a/hgext/evolve.py
> +++ b/hgext/evolve.py
> @@ -3257,29 +3257,57 @@ def metaedit(ui, repo, *revs, **opts):
>               if commitopts.get('message') or commitopts.get('logfile'):
>                   commitopts['edit'] = False
>               else:
> -                if opts['fold']:
> -                    msgs = ["HG: This is a fold of %d changesets." % len(allctx)]
> -                    msgs += ["HG: Commit message of changeset %s.\n\n%s\n" %
> -                             (c.rev(), c.description()) for c in allctx]
> -                else:
> -                    msgs = [head.description()]
> -                commitopts['message'] =  "\n".join(msgs)

Can you put this logic move into a separate commit? The idea that this 
logic can move down the file without affecting behavior is non-obvious.

>                   commitopts['edit'] = True

Is this line now duplicated above and below the else? Why not remove the 
if/else entirely then?
>   
>               # TODO: if the author and message are the same, don't create a new
>               # hash. Right now we create a new hash because the date can be
>               # different.
> -            newid, created = rewrite(repo, root, allctx, head,
> -                                     [root.p1().node(), root.p2().node()],
> -                                     commitopts=commitopts)
> -            if created:
> -                if p1.rev() in revs:
> -                    newp1 = newid
> -                phases.retractboundary(repo, tr, targetphase, [newid])
> -                obsolete.createmarkers(repo, [(ctx, (repo[newid],))
> -                                              for ctx in allctx])
> +            if opts['fold']:
> +                if commitopts['edit']:
> +                    msgs = ["HG: This is a fold of %d changesets." %
> +                            len(allctx)]
> +                    msgs += ["HG: Commit message of changeset %s.\n\n%s\n" %
> +                             (c.rev(), c.description()) for c in allctx]
> +                    commitopts['message'] = "\n".join(msgs)
> +
> +                newid, created = rewrite(repo, root, allctx, head,
> +                                         [root.p1().node(), root.p2().node()],
> +                                         commitopts=commitopts)
> +                if created:
> +                    if p1.rev() in revs:
> +                        newp1 = newid
> +                    phases.retractboundary(repo, tr, targetphase, [newid])
> +                    obsolete.createmarkers(repo, [(ctx, (repo[newid],))
> +                                                  for ctx in allctx])
> +                else:
> +                    ui.status(_("nothing changed\n"))
>               else:
> -                ui.status(_("nothing changed\n"))
> +                replacemap = {}
> +                # we need topological order
> +                allctx = sorted(allctx, key=lambda c: c.rev())
> +                for c in allctx:
> +                    if commitopts['edit']:
> +                        commitopts['message'] = \
> +                            "HG: Commit message of changeset %s\n%s" %\
> +                            (str(c), c.description())
> +                    bases = [
> +                        replacemap.get(c.p1().node(), c.p1().node()),
> +                        replacemap.get(c.p2().node(), c.p2().node()),
> +                    ]
> +                    newid, created = metarewrite(repo, c, bases,
> +                                                 commitopts=commitopts)
> +                    if created:
> +                        replacemap[c.node()] = newid
> +
> +                if p1.node() in replacemap:
> +                    newp1 = replacemap[p1.node()]

Why not use replacemap.get() here?

> +                if len(replacemap) > 0:
> +                    obsolete.createmarkers(repo, [(repo[old], (repo[new],))
> +                        for old, new in replacemap.iteritems()])
> +                    # TODO: set poroper phase boundaries

This shouldn't apply, should it? We can only edit non-public commits, so 
the boundaries shouldn't shift?

> +                else:
> +                    ui.status(_("nothing changed\n"))
> +
>               tr.close()
>           finally:
>               tr.release()

Patches 1, 3, and 4 look good to me. This one I think can be split and 
sub-parts can include additional clean-ups as noted inline.


More information about the Mercurial-devel mailing list