[PATCH] histedit: handle obsolete commits unknown to histedit state (issue4800)

[Pierre-Yves David]

[I would says it not freeze worthy (because it is a lot of code and 
affects experimental feature), wait until january to resend. But I had 
this review half written in my draft directory so I'm sending it]

On 01/18/2016 04:02 AM, Kostia Balytskyi wrote:
> # HG changeset patch
> # User Kostia Balytskyi <ikostia at fb.com>
> # Date 1453118372 0
> #      Mon Jan 18 11:59:32 2016 +0000
> # Node ID 289562ec0ef6a732c54900842224a04932009d05
> # Parent  443848eece189002c542339dc1cf84f49a94c824
> histedit: handle obsolete commits unknown to histedit state (issue4800)
>
> This fix 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,32 @@
>       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.
> +    """
> +    if not obsolete.isenabled(repo, obsolete.createmarkersopt):
> +        # no obsoletion is enabled, no successors should exist
> +        return nodes
> +
> +    result = []

I'm not sure why we takes a list in input and produce a list in output. 
This is the kind of data that we usually want accurate. So either a 
single node in entry and a list as a return or a list in entry and a 
dict of list as output?

> +    for cl in nodes:

naming nits: cl is a command variable name for changelog. If you have 
nodes in this list, use "n".

> +        succset = obsolete.successorssets(repo, cl)
> +        if len(succset) > 1:
> +            msg = _("changeset %s split or divergent,"
> +                    " cannot determine unique successor")

1) That's not a split. a split is one successors set with multiple 
elements (there is one rewrite path, leading to multiple changesets). 
divergence is multiple successors sets (thre is multiple rewrite paths)

2) If we could avoid aborting and just print a warning that would be 
nice. Plain abort is rarely a good user experience. Moreover, your 
specific targeted instalation have all divergence display, warning and 
resolution disabled, so your code might hit that path without the user 
behind able to get anything out of the situation.

2.a) if we run it at the right time (right after commands/interruption) 
there is a good chance we can sort out the divergence by looking at what 
branch is actually new.

> +            hint =_("solve the issue and run histedit --continue"
> +                    " or abort with histedit --abort")
> +            raise error.Abort(msg % node.hex(cl), hint=hint)
> +        elif len(succset) == 1:
> +            # non-divergent successor set
> +            result += succset[0]

successors sets can contains multiple entry (split). In the simple case, 
they are on the same branch and you can pick the tipmost. In other 
cases, this is more tricky. See evolve destination logic for details.

> +        else:
> +            # changeset has been pruned, nothing needs to be done
> +            pass
> +    return result
>
>   def processreplacement(state):

Why are we doing this at the end only and not a long the line. We should 
probably just look at obsmarker for any missing/new nodes after a 
--continue or command run instead of doing a blind a global reprocessing 
at the very end.

>       """process the list of replacements to return
> @@ -1408,11 +1434,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)
>

We'll want a couple if lines of comments to explain

> +    nonobsoletenew = nonobsoletesuccessors(state.repo, new)
>       # computed topmost element (necessary for bookmark)
> -    if new:
> -        newtopmost = sorted(new, key=state.repo.changelog.rev)[-1]
> +    if nonobsoletenew:
> +        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 +1450,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"""
> diff --git a/tests/test-histedit-edit.t b/tests/test-histedit-edit.t
> --- a/tests/test-histedit-edit.t
> +++ b/tests/test-histedit-edit.t
> @@ -477,3 +477,33 @@
>     #  f, fold = use commit, but combine it with the one above
>     #  r, roll = like fold, but discard this commit's description
>     #
> +
> +Ensure that creating obsole revisions while editing commits does not break histedit
> +
> +  $ hg init boo
> +  $ cd boo
> +  $ echo a > aaaa
> +  $ hg ci -Am a
> +  adding aaaa
> +  $ echo a > bbbb
> +  $ echo a > cccc
> +  $ echo a > cccc
> +  $ hg ci -Am b
> +  adding bbbb
> +  adding cccc
> +  $ echo a > dddd
> +  $ hg ci -Am c
> +  adding dddd
> +  $ echo "pick `hg log -r -1 -T '{node|short}'`" > plan
> +  $ echo "pick `hg log -r -3 -T '{node|short}'`" >> plan
> +  $ echo "edit `hg log -r -2 -T '{node|short}'`" >> plan
> +  $ hg histedit -qr 'all()' --commands plan
> +  Editing \([0-9a-f]{12}\), you may commit or record as needed now. (re)
> +  (hg histedit --continue to resume)
> +  [1]
> +  $ hg st
> +  A bbbb
> +  A cccc
> +  ? plan
> +  $ hg commit --amend bbbb
> +  saved backup bundle to $TESTTMP/r0/boo/.hg/strip-backup/*-amend-backup.hg (glob)

-- 
Pierre-Yves David