Patchwork histedit: handle obsolete commits unknown to histedit state

# 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.

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