Patchwork [v2] histedit: make histedit aware of obsoletions not stored in state (issue4800)

login
register
mail settings
Submitter Kostia Balytskyi
Date Feb. 22, 2016, 2:50 p.m.
Message ID <426bfeb7144bb8ad34ae.1456152652@android-802ec2ba4da5933b.dhcp.thefacebook.com>
Download mbox | patch
Permalink /patch/13292/
State Rejected
Headers show

Comments

Kostia Balytskyi - Feb. 22, 2016, 2:50 p.m.
# HG changeset patch
# User Kostia Balytskyi <ikostia@fb.com>
# Date 1456152481 0
#      Mon Feb 22 14:48:01 2016 +0000
# Node ID 426bfeb7144bb8ad34aefa003f39be15217ccc54
# Parent  28e1694ca60056d609ae2c8e0ad5cb2891416ea3
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.
Pierre-Yves David - Feb. 22, 2016, 3:31 p.m.
On 02/22/2016 03:50 PM, Kostia Balytskyi wrote:
> # HG changeset patch
> # User Kostia Balytskyi <ikostia@fb.com>
> # Date 1456152481 0
> #      Mon Feb 22 14:48:01 2016 +0000
> # Node ID 426bfeb7144bb8ad34aefa003f39be15217ccc54
> # Parent  28e1694ca60056d609ae2c8e0ad5cb2891416ea3
> 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

(I missed that on the first pass, but "obsoletion change" is not a 
wording we used anywhere. You can probably replace it by "obsolescence 
marker" here.

> 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/hg b/hg
> --- a/hg
> +++ b/hg
> @@ -39,5 +39,4 @@
>
>   for fp in (sys.stdin, sys.stdout, sys.stderr):
>       mercurial.util.setbinary(fp)
> -
>   mercurial.dispatch.run()
> diff --git a/hgext/histedit.py b/hgext/histedit.py
> --- a/hgext/histedit.py
> +++ b/hgext/histedit.py
> @@ -1386,6 +1386,34 @@
>                   hint=_('use "drop %s" to discard, see also: '
>                          '"hg help -e histedit.config"') % missing[0][:12])
>
> +def adjustreplacementsfrommarkers(repo, allsuccs, replaced, fullmapping):
> +    """Adjusts replacement structures with data from obsoletion markers
> +
> +    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
> +
> +    succstocheck = list(allsuccs)
> +    while succstocheck:

I suspect that starting with allsuccs will have you miss the case where 
an original node disappear that way. Are you testing this case?

> +        n = succstocheck.pop()
> +        ctx = repo.unfiltered()[n]

You should cache 'repo.unfiltered()' as "unfi" outside the loop.

> +        firstiteration = True
> +        for marker in obsolete.successormarkers(ctx):
> +            for nsucc in marker.succnodes():
> +                # nsucc is a successor of n
> +                if firstiteration:
> +                    # this is the first time we see that node n
> +                    # was replaced by something
> +                    firstiteration = False
> +                    replaced.add(n)

'replaced' is a set so it can bear with 'n' being added multiple time. 
I'm either missing something of 'firstiteration' is unneeded here.

> +                if nsucc not in allsuccs:
> +                    # save nsucc for further investigation
> +                    allsuccs.add(nsucc)
> +                    succstocheck.append(nsucc)
> +                fullmapping.setdefault(n, set()).add(nsucc)

note: nsucc maybe be locally unknown. As we unconditionnaly try to build 
a ctx out of it. this will probably blow up if this ever happens. We 
should protect the code agains this case.

>   def processreplacement(state):
>       """process the list of replacements to return
>
> @@ -1402,6 +1430,7 @@
>           allsuccs.update(rep[1])
>           replaced.add(rep[0])
>           fullmapping.setdefault(rep[0], set()).update(rep[1])
> +    adjustreplacementsfrommarkers(state.repo, allsuccs, replaced, fullmapping)
>       new = allsuccs - replaced
>       tmpnodes = allsuccs & replaced
>       # Reduce content fullmapping into direct relation between original nodes
> diff --git a/tests/test-histedit-obsolete.t b/tests/test-histedit-obsolete.t
> --- a/tests/test-histedit-obsolete.t
> +++ b/tests/test-histedit-obsolete.t
> @@ -14,6 +14,36 @@
>     > rebase=
>     > EOF
>
> +Test that histedit learns about obsoletions not stored in histedit state
> +  $ hg init boo
> +  $ cd boo
> +  $ echo a > a
> +  $ hg ci -Am a
> +  adding a
> +  $ echo a > b
> +  $ echo a > c
> +  $ echo a > c
> +  $ hg ci -Am b
> +  adding b
> +  adding c
> +  $ echo a > d
> +  $ hg ci -Am c
> +  adding d
> +  $ echo "pick `hg log -r 0 -T '{node|short}'`" > plan
> +  $ echo "pick `hg log -r 2 -T '{node|short}'`" >> plan
> +  $ echo "edit `hg log -r 1 -T '{node|short}'`" >> plan
> +  $ hg histedit -r 'all()' --commands plan
> +  Editing (1b2d564fad96), you may commit or record as needed now.
> +  (hg histedit --continue to resume)
> +  [1]
> +  $ hg st
> +  A b
> +  A c
> +  ? plan
> +  $ hg commit --amend b
> +  $ hg histedit --continue
> +  $ cd ..
> +
>     $ hg init base
>     $ cd base
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Pierre-Yves David - Feb. 22, 2016, 3:40 p.m.
(urg previous email sent too fast)

On 02/22/2016 03:50 PM, Kostia Balytskyi wrote:
> # HG changeset patch
> # User Kostia Balytskyi <ikostia@fb.com>
> # Date 1456152481 0
> #      Mon Feb 22 14:48:01 2016 +0000
> # Node ID 426bfeb7144bb8ad34aefa003f39be15217ccc54
> # Parent  28e1694ca60056d609ae2c8e0ad5cb2891416ea3
> 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/hg b/hg
> --- a/hg
> +++ b/hg
> @@ -39,5 +39,4 @@
>
>   for fp in (sys.stdin, sys.stdout, sys.stderr):
>       mercurial.util.setbinary(fp)
> -
>   mercurial.dispatch.run()
> diff --git a/hgext/histedit.py b/hgext/histedit.py
> --- a/hgext/histedit.py
> +++ b/hgext/histedit.py
> @@ -1386,6 +1386,34 @@
>                   hint=_('use "drop %s" to discard, see also: '
>                          '"hg help -e histedit.config"') % missing[0][:12])
>
> +def adjustreplacementsfrommarkers(repo, allsuccs, replaced, fullmapping):
> +    """Adjusts replacement structures with data from obsoletion markers
> +
> +    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
> +
> +    succstocheck = list(allsuccs)
> +    while succstocheck:
> +        n = succstocheck.pop()
> +        ctx = repo.unfiltered()[n]
> +        firstiteration = True
> +        for marker in obsolete.successormarkers(ctx):
> +            for nsucc in marker.succnodes():
> +                # nsucc is a successor of n
> +                if firstiteration:
> +                    # this is the first time we see that node n
> +                    # was replaced by something
> +                    firstiteration = False
> +                    replaced.add(n)

This update to replacement is actually getting me a bit surprised. cf 
below ?

> +                if nsucc not in allsuccs:
> +                    # save nsucc for further investigation
> +                    allsuccs.add(nsucc)
> +                    succstocheck.append(nsucc)
> +                fullmapping.setdefault(n, set()).add(nsucc)
> +
>   def processreplacement(state):
>       """process the list of replacements to return
>
> @@ -1402,6 +1430,7 @@
>           allsuccs.update(rep[1])
>           replaced.add(rep[0])
>           fullmapping.setdefault(rep[0], set()).update(rep[1])
> +    adjustreplacementsfrommarkers(state.repo, allsuccs, replaced, fullmapping)

We shouln't we read data related to marker before we start any 
processing? so that processreplacement just read and process full 
mapping in "on block" ?

Patch

diff --git a/hg b/hg
--- a/hg
+++ b/hg
@@ -39,5 +39,4 @@ 
 
 for fp in (sys.stdin, sys.stdout, sys.stderr):
     mercurial.util.setbinary(fp)
-
 mercurial.dispatch.run()
diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -1386,6 +1386,34 @@ 
                 hint=_('use "drop %s" to discard, see also: '
                        '"hg help -e histedit.config"') % missing[0][:12])
 
+def adjustreplacementsfrommarkers(repo, allsuccs, replaced, fullmapping):
+    """Adjusts replacement structures with data from obsoletion markers
+
+    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
+
+    succstocheck = list(allsuccs)
+    while succstocheck:
+        n = succstocheck.pop()
+        ctx = repo.unfiltered()[n]
+        firstiteration = True
+        for marker in obsolete.successormarkers(ctx):
+            for nsucc in marker.succnodes():
+                # nsucc is a successor of n
+                if firstiteration:
+                    # this is the first time we see that node n
+                    # was replaced by something
+                    firstiteration = False
+                    replaced.add(n)
+                if nsucc not in allsuccs:
+                    # save nsucc for further investigation
+                    allsuccs.add(nsucc)
+                    succstocheck.append(nsucc)
+                fullmapping.setdefault(n, set()).add(nsucc)
+
 def processreplacement(state):
     """process the list of replacements to return
 
@@ -1402,6 +1430,7 @@ 
         allsuccs.update(rep[1])
         replaced.add(rep[0])
         fullmapping.setdefault(rep[0], set()).update(rep[1])
+    adjustreplacementsfrommarkers(state.repo, allsuccs, replaced, fullmapping)
     new = allsuccs - replaced
     tmpnodes = allsuccs & replaced
     # Reduce content fullmapping into direct relation between original nodes
diff --git a/tests/test-histedit-obsolete.t b/tests/test-histedit-obsolete.t
--- a/tests/test-histedit-obsolete.t
+++ b/tests/test-histedit-obsolete.t
@@ -14,6 +14,36 @@ 
   > rebase=
   > EOF
 
+Test that histedit learns about obsoletions not stored in histedit state
+  $ hg init boo
+  $ cd boo
+  $ echo a > a
+  $ hg ci -Am a
+  adding a
+  $ echo a > b
+  $ echo a > c
+  $ echo a > c
+  $ hg ci -Am b
+  adding b
+  adding c
+  $ echo a > d
+  $ hg ci -Am c
+  adding d
+  $ echo "pick `hg log -r 0 -T '{node|short}'`" > plan
+  $ echo "pick `hg log -r 2 -T '{node|short}'`" >> plan
+  $ echo "edit `hg log -r 1 -T '{node|short}'`" >> plan
+  $ hg histedit -r 'all()' --commands plan
+  Editing (1b2d564fad96), you may commit or record as needed now.
+  (hg histedit --continue to resume)
+  [1]
+  $ hg st
+  A b
+  A c
+  ? plan
+  $ hg commit --amend b
+  $ hg histedit --continue
+  $ cd ..
+
   $ hg init base
   $ cd base