Submitter | Kostia Balytskyi |
---|---|
Date | Jan. 14, 2016, 7:19 p.m. |
Message ID | <5a36c6cdb955ffb1f8b4.1452799181@ikostia-mbp.dhcp.thefacebook.com> |
Download | mbox | patch |
Permalink | /patch/12763/ |
State | Changes Requested |
Delegated to: | Pierre-Yves David |
Headers | show |
Comments
On Thu, 14 Jan 2016 11:19:41 -0800, Kostia Balytskyi wrote: > # HG changeset patch > # User Kostia Balytskyi <ikostia@fb.com> > # Date 1452727707 28800 > # Wed Jan 13 15:28:27 2016 -0800 > # Node ID 5a36c6cdb955ffb1f8b4c2f26d369b3d331c0492 > # Parent 443848eece189002c542339dc1cf84f49a94c824 > histedit: handle obsolete commits unknown to histedit state > > This fix is intended to solve issue4800. It takes sets of > final and new commits from histedit's state and replaces > them with their successor sets. The logic seems good, but I know little about histedit, so I only checked coding errors. > --- a/hgext/histedit.py > +++ b/hgext/histedit.py > @@ -1362,6 +1362,27 @@ > tmpnodes = allsuccs & replaced > return newnodes, tmpnodes > > +def nonobsoletesuccessors(repo, nodes): > + """find all non-obsolete successors for each of the nodes > + > + This fails if any of the nodes has more than one successor > + set, e.g. if it diverged at some point and then became obsolete. > + """ > + result = [] > + for cl in nodes: > + succset = obsolete.successorssets(repo, cl) > + if len(succset) > 1: > + # successorsset for diverged changeset > + # weird situation that should not happen while > + # editing history, indicates an error > + msg = _("diverged obsolete changeset found " + > + "among nodes in histedit: %s") _() shouldn't contain expression. '+' isn't necessary because string literals are concatenated at parsing time. > # computed topmost element (necessary for bookmark) > if new: > - newtopmost = sorted(new, key=state.repo.changelog.rev)[-1] > + nonobsoletenew = nonobsoletesuccessors(state.repo, new) > + newtopmost = sorted(nonobsoletenew, key=state.repo.changelog.rev)[-1] > elif not final: > # Nothing rewritten at all. we won't need `newtopmost` > # It is the same as `oldtopmost` and `processreplacement` know it > @@ -1422,7 +1445,7 @@ > r = state.repo.changelog.rev > newtopmost = state.repo[sorted(final, key=r)[0]].p1().node() > > - return final, tmpnodes, new, newtopmost > + return final, tmpnodes, nonobsoletenew, newtopmost nonobsoletenew isn't initialized if new is empty.
Hi, We typically put the issue # in the first line of the commit message like: topic <msg> (issue4800) > On Jan 14, 2016, at 11:19 AM, Kostia Balytskyi <ikostia@fb.com> wrote: > > # HG changeset patch > # User Kostia Balytskyi <ikostia@fb.com> > # Date 1452727707 28800 > # Wed Jan 13 15:28:27 2016 -0800 > # Node ID 5a36c6cdb955ffb1f8b4c2f26d369b3d331c0492 > # Parent 443848eece189002c542339dc1cf84f49a94c824 > histedit: handle obsolete commits unknown to histedit state > > This fix is intended to solve issue4800. It takes sets of > final and new commits from histedit's state and replaces > them with their successor sets. > > diff --git a/hgext/histedit.py b/hgext/histedit.py > --- a/hgext/histedit.py > +++ b/hgext/histedit.py > @@ -1362,6 +1362,27 @@ > tmpnodes = allsuccs & replaced > return newnodes, tmpnodes > > +def nonobsoletesuccessors(repo, nodes): > + """find all non-obsolete successors for each of the nodes > + > + This fails if any of the nodes has more than one successor > + set, e.g. if it diverged at some point and then became obsolete. > + """ We probably want to exit early is marker creation is disabled. See obsolete.isenabled. > + result = [] > + for cl in nodes: > + succset = obsolete.successorssets(repo, cl) > + if len(succset) > 1: > + # successorsset for diverged changeset > + # weird situation that should not happen while > + # editing history, indicates an error This is not a weird situation that should not happen, it is more that we don't really know what to do about it. I would say "split or divergent changeset, can't determine the successor to rebase on". There is no need for a comment if the message is explicit. How about printing the changeset description and say something more descriptive like: msg = _("changeset split or divergent, cannot determine unique successor") hint =_("solve the issue and run histedit --continue or abort with histedit --abort") raise error.Abort(msg, hint=hint) I am not very happy with these messages though as it does not explain how to solve the issue and we can't really do that in one line. Pierre-Yves, do you have a better idea for the phrasing? > + msg = _("diverged obsolete changeset found " + > + "among nodes in histedit: %s") > + raise error.Abort(msg % node.hex(cl)) > + elif len(succset) == 1: > + # non-divergent successor set > + result += succset[0] > + # else: changeset has been pruned, nothing needs to be done I would prefer this form: else: # comment pass Did you test this case too? Could you add new tests for these scenarios? > + return result > > def processreplacement(state): > """process the list of replacements to return > @@ -1408,11 +1429,13 @@ > # turn `final` into list (topologically sorted) > nm = state.repo.changelog.nodemap > for prec, succs in final.items(): > + succs = nonobsoletesuccessors(state.repo, succs) > final[prec] = sorted(succs, key=nm.get) > > # computed topmost element (necessary for bookmark) > if new: > - newtopmost = sorted(new, key=state.repo.changelog.rev)[-1] > + nonobsoletenew = nonobsoletesuccessors(state.repo, new) > + newtopmost = sorted(nonobsoletenew, key=state.repo.changelog.rev)[-1] > elif not final: > # Nothing rewritten at all. we won't need `newtopmost` > # It is the same as `oldtopmost` and `processreplacement` know it > @@ -1422,7 +1445,7 @@ > r = state.repo.changelog.rev > newtopmost = state.repo[sorted(final, key=r)[0]].p1().node() > > - return final, tmpnodes, new, newtopmost > + return final, tmpnodes, nonobsoletenew, new topmost > > def movebookmarks(ui, repo, mapping, oldtopmost, newtopmost): > """Move bookmark from old to newly created node""" > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@selenic.com > https://selenic.com/mailman/listinfo/mercurial-devel Thanks, Laurent
Patch
diff --git a/hgext/histedit.py b/hgext/histedit.py --- a/hgext/histedit.py +++ b/hgext/histedit.py @@ -1362,6 +1362,27 @@ tmpnodes = allsuccs & replaced return newnodes, tmpnodes +def nonobsoletesuccessors(repo, nodes): + """find all non-obsolete successors for each of the nodes + + This fails if any of the nodes has more than one successor + set, e.g. if it diverged at some point and then became obsolete. + """ + result = [] + for cl in nodes: + succset = obsolete.successorssets(repo, cl) + if len(succset) > 1: + # successorsset for diverged changeset + # weird situation that should not happen while + # editing history, indicates an error + msg = _("diverged obsolete changeset found " + + "among nodes in histedit: %s") + raise error.Abort(msg % node.hex(cl)) + elif len(succset) == 1: + # non-divergent successor set + result += succset[0] + # else: changeset has been pruned, nothing needs to be done + return result def processreplacement(state): """process the list of replacements to return @@ -1408,11 +1429,13 @@ # turn `final` into list (topologically sorted) nm = state.repo.changelog.nodemap for prec, succs in final.items(): + succs = nonobsoletesuccessors(state.repo, succs) final[prec] = sorted(succs, key=nm.get) # computed topmost element (necessary for bookmark) if new: - newtopmost = sorted(new, key=state.repo.changelog.rev)[-1] + nonobsoletenew = nonobsoletesuccessors(state.repo, new) + newtopmost = sorted(nonobsoletenew, key=state.repo.changelog.rev)[-1] elif not final: # Nothing rewritten at all. we won't need `newtopmost` # It is the same as `oldtopmost` and `processreplacement` know it @@ -1422,7 +1445,7 @@ r = state.repo.changelog.rev newtopmost = state.repo[sorted(final, key=r)[0]].p1().node() - return final, tmpnodes, new, newtopmost + return final, tmpnodes, nonobsoletenew, newtopmost def movebookmarks(ui, repo, mapping, oldtopmost, newtopmost): """Move bookmark from old to newly created node"""