Patchwork [5,of,5] histedit: do not stay on a cleaned nodes on abort

login
register
mail settings
Submitter Pierre-Yves David
Date Aug. 3, 2015, 9:25 p.m.
Message ID <ab1f4da158c6950ed6b8.1438637127@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/10098/
State Accepted
Headers show

Comments

Pierre-Yves David - Aug. 3, 2015, 9:25 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1438607585 25200
#      Mon Aug 03 06:13:05 2015 -0700
# Node ID ab1f4da158c6950ed6b8eb645c5a634ec169f55a
# Parent  ec0875fa682acd025a41471438c64041a37a2f82
histedit: do not stay on a cleaned nodes on abort

There is case where nodes are neither in tmpnodes nor leaf but still get
removed. For example, if you used the "edit" action, made a commit and run
--abort. The commit you made is not tracked by histedit, yet it will likely be
cleaned up with its parent. The commit may not tracked because no replacements
computations are done in the --abort case.
Laurent Charignon - Aug. 3, 2015, 11:29 p.m.
This series looks good to me and seems to fix the issues that we have been seeing using histedit with evolve.
I talked to you offline about a test case that used to not work and that this series is fixing, let's add it to make sure that we don't regress.

Thanks,

Laurent

> On Aug 3, 2015, at 2:25 PM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> 
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1438607585 25200
> #      Mon Aug 03 06:13:05 2015 -0700
> # Node ID ab1f4da158c6950ed6b8eb645c5a634ec169f55a
> # Parent  ec0875fa682acd025a41471438c64041a37a2f82
> histedit: do not stay on a cleaned nodes on abort
> 
> There is case where nodes are neither in tmpnodes nor leaf but still get
> removed. For example, if you used the "edit" action, made a commit and run
> --abort. The commit you made is not tracked by histedit, yet it will likely be
> cleaned up with its parent. The commit may not tracked because no replacements
> computations are done in the --abort case.
> 
> diff --git a/hgext/histedit.py b/hgext/histedit.py
> --- a/hgext/histedit.py
> +++ b/hgext/histedit.py
> @@ -789,11 +789,11 @@ def _histedit(ui, repo, state, *freeargs
>             changegroup.addchangegroup(repo, gen, 'histedit',
>                                        'bundle:' + backupfile)
>             os.remove(backupfile)
> 
>         # check whether we should update away
> -        if repo.unfiltered().revs('parents() and (%n  or %ln)',
> +        if repo.unfiltered().revs('parents() and (%n  or %ln::)',
>                                   state.parentctxnode, leafs | tmpnodes):
>             hg.clean(repo, state.topmost)
>         cleanupnode(ui, repo, 'created', tmpnodes)
>         cleanupnode(ui, repo, 'temp', leafs)
>         state.clear()
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - Aug. 3, 2015, 11:35 p.m.
On 08/03/2015 04:29 PM, Laurent Charignon wrote:
> This series looks good to me and seems to fix the issues that we have been seeing using histedit with evolve.
> I talked to you offline about a test case that used to not work and that this series is fixing, let's add it to make sure that we don't regress.

The test case required the histedit extensions use at Facebook right? 
and so the test case should live there. Or are we talking about 
something else?
Laurent Charignon - Aug. 4, 2015, 12:28 a.m.
Oh yeah, sorry I forgot that we would need that extension to add the test.
You can proceed then.

Thanks,

Laurent

> On Aug 3, 2015, at 4:35 PM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> 
> 
> 
> On 08/03/2015 04:29 PM, Laurent Charignon wrote:
>> This series looks good to me and seems to fix the issues that we have been seeing using histedit with evolve.
>> I talked to you offline about a test case that used to not work and that this series is fixing, let's add it to make sure that we don't regress.
> 
> The test case required the histedit extensions use at Facebook right? and so the test case should live there. Or are we talking about something else?
> 
> -- 
> Pierre-Yves David
Martin von Zweigbergk - Aug. 5, 2015, 5:03 p.m.
This looks good and simple enough even for someone not familiar with
histedit internals that I dare push it to the clowncopter (done). Thanks!

On Mon, Aug 3, 2015 at 5:29 PM Laurent Charignon <lcharignon@fb.com> wrote:

> Oh yeah, sorry I forgot that we would need that extension to add the test.
> You can proceed then.
>
> Thanks,
>
> Laurent
>
> > On Aug 3, 2015, at 4:35 PM, Pierre-Yves David <
> pierre-yves.david@ens-lyon.org> wrote:
> >
> >
> >
> > On 08/03/2015 04:29 PM, Laurent Charignon wrote:
> >> This series looks good to me and seems to fix the issues that we have
> been seeing using histedit with evolve.
> >> I talked to you offline about a test case that used to not work and
> that this series is fixing, let's add it to make sure that we don't regress.
> >
> > The test case required the histedit extensions use at Facebook right?
> and so the test case should live there. Or are we talking about something
> else?
> >
> > --
> > Pierre-Yves David
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
>

Patch

diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -789,11 +789,11 @@  def _histedit(ui, repo, state, *freeargs
             changegroup.addchangegroup(repo, gen, 'histedit',
                                        'bundle:' + backupfile)
             os.remove(backupfile)
 
         # check whether we should update away
-        if repo.unfiltered().revs('parents() and (%n  or %ln)',
+        if repo.unfiltered().revs('parents() and (%n  or %ln::)',
                                   state.parentctxnode, leafs | tmpnodes):
             hg.clean(repo, state.topmost)
         cleanupnode(ui, repo, 'created', tmpnodes)
         cleanupnode(ui, repo, 'temp', leafs)
         state.clear()