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

Mateusz Kwapich mitrandir at fb.com
Mon Dec 5 12:22:27 EST 2016


Excerpts from Ryan McElroy's message of 2016-12-05 13:14:58 +0000:
> 
> 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.

I'll try to split it in the next version of this series.
> 
> >                   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?
I'll do it.
> 
> > +                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?

It's about preservation of the 'secret' phase.
> 
> > +                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