[PATCH 2 of 2] histedit: make histedit aware of obsoletions not stored in state (issue4800)

[Pierre-Yves David]

On 02/16/2016 10:29 PM, Kostia Balytskyi wrote:
> # HG changeset patch
> # User Kostia Balytskyi <ikostia at fb.com>
> # Date 1455658079 0
> #      Tue Feb 16 21:27:59 2016 +0000
> # Node ID c4c7a4ca3132fac82a7e6f3c74a3ff247a309cc6
> # Parent  34ffbe866b265b837c8a592f95293a53fa310dce
> histedit: make histedit aware of obsoletions not stored in state (issue4800)
>
> Before this change, when histedit exited to interactive session (during edit
> command for example), user could introduce obsoletion changes that would not
> be known to histedit. For example, user could've amended one of the commits
> (introducing an obsoletion change). The fact of this amendment would not be
> stored in histedit's state file and later, when histedit would try to process
> all the replacements that happened, one of the final successors (in its opinion)
> would turn out to be hidden. This behavior is described in issue4800. This
> commit fixes it.
>
> diff --git a/hgext/histedit.py b/hgext/histedit.py
> --- a/hgext/histedit.py
> +++ b/hgext/histedit.py
> @@ -1386,6 +1386,31 @@
>                   hint=_('use "drop %s" to discard, see also: '
>                          '"hg help -e histedit.config"') % missing[0][:12])
>
> +def adjustreplacementsfrommarkers(repo, allsuccs, replaced, fullmapping):
> +    """Adjusts `replaced`, `allsuccs` and `fullmapping` structures
> +    with data from obsoletion markers

small nit: in pep8, docstring are defined a bit like our commit message

   single line summary

   Longer description.
   Possibly on multiple lines.

I personally thing it is a good idea.

> +    These structures are originally generated only based on
> +    histedit state and do not account for changes that are
> +    not recorded there. This function fixes that"""
> +    if not obsolete.isenabled(repo, obsolete.createmarkersopt):
> +        return
> +
> +    cache = {}
> +    originalsuccs = set(allsuccs)

smallnits: "original" got me confused as it is a word I could have used 
for "original node before histedit". maybe says "oldsuccss"

> +    for n in originalsuccs:
> +        finals = set()
> +        finals.update(*obsolete.successorssets(repo, n, cache=cache))
> +        if len(finals) == 1 and n in finals:
> +            continue

You can probably get the same data more efficiently by looking if the 
"n" have any obsmarkers on it.

> +        allnsuccs = set(obsolete.allsuccessors(repo.obsstore, [n]))
> +        allsuccs.update(allnsuccs)
> +        replaced.update(allnsuccs - finals)

That section could probably use some comment. The logic seems right to 
me. But I had to got double read the original code to figure that out.

> +        fullmapping.setdefault(n, set()).update(finals)

Same here. At that point, the fullmapping seems to have been reduced to 
x → final successors (while not containing that data for most of the 
run). We should point that out to make sure future update understand 
this and don't introduce error when editing/changing its purpose.

> +        if n in fullmapping[n]:
> +            fullmapping[n].remove(n)

You could discard 'n' from final before updating fullmapping. That would 
avoid this extra step.


Would you send a V2 with extra comments?

Cheers,

-- 
Pierre-Yves David