[PATCH] histedit: handle obsolete commits unknown to histedit state

[Laurent Charignon]

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 at fb.com> wrote:
> 
> # HG changeset patch
> # User Kostia Balytskyi <ikostia at 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 at selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel


Thanks,

Laurent