Patchwork revert: add a XXX about rename tracking

login
register
mail settings
Submitter Pierre-Yves David
Date Aug. 15, 2014, 5:10 p.m.
Message ID <b12294b39da84f2dad4d.1408122606@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/5435/
State Accepted
Headers show

Comments

Pierre-Yves David - Aug. 15, 2014, 5:10 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1403625580 -3600
#      Tue Jun 24 16:59:40 2014 +0100
# Node ID b12294b39da84f2dad4d416161f5e48bb9966da3
# Parent  8dda6f6ff564d8fe6ac7b8ce4c74eb9bfb5de14a
revert: add a XXX about rename tracking

We check for rename information in the dirstate this is probably not enough to
preserve this behavior when using an explicit target revs.

I just spotted this while working on this code, but this is outside the scope
of my series so I'm just adding a comment to highlight this suspicious
situation.
Augie Fackler - Aug. 18, 2014, 10:31 p.m.
On Fri, Aug 15, 2014 at 10:10:06AM -0700, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1403625580 -3600
> #      Tue Jun 24 16:59:40 2014 +0100
> # Node ID b12294b39da84f2dad4d416161f5e48bb9966da3
> # Parent  8dda6f6ff564d8fe6ac7b8ce4c74eb9bfb5de14a
> revert: add a XXX about rename tracking

Queued. BTW when I queued this with 8dda as my working copy, 'hg
import --obsolete' gave me a self-obsoleting change again. It took
some fighting to get out of the bad state.

Would you object to a change to the obsstore that banned creation of
cycles?

>
> We check for rename information in the dirstate this is probably not enough to
> preserve this behavior when using an explicit target revs.
>
> I just spotted this while working on this code, but this is outside the scope
> of my series so I'm just adding a comment to highlight this suspicious
> situation.
>
> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -2422,10 +2422,11 @@ def revert(ui, repo, ctx, parents, *pats
>
>          # if f is a rename, update `names` to also revert the source
>          cwd = repo.getcwd()
>          for f in dsadded:
>              src = repo.dirstate.copied(f)
> +            # XXX should we check for rename down to target node?
>              if src and src not in names and repo.dirstate[src] == 'r':
>                  dsremoved.add(src)
>                  names[src] = (repo.pathto(src, cwd), True)
>
>          ## computation of the action to performs on `names` content.
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Matt Mackall - Aug. 18, 2014, 11:05 p.m.
On Mon, 2014-08-18 at 18:31 -0400, Augie Fackler wrote:
> On Fri, Aug 15, 2014 at 10:10:06AM -0700, Pierre-Yves David wrote:
> > # HG changeset patch
> > # User Pierre-Yves David <pierre-yves.david@fb.com>
> > # Date 1403625580 -3600
> > #      Tue Jun 24 16:59:40 2014 +0100
> > # Node ID b12294b39da84f2dad4d416161f5e48bb9966da3
> > # Parent  8dda6f6ff564d8fe6ac7b8ce4c74eb9bfb5de14a
> > revert: add a XXX about rename tracking
> 
> Queued. BTW when I queued this with 8dda as my working copy, 'hg
> import --obsolete' gave me a self-obsoleting change again. It took
> some fighting to get out of the bad state.
> 
> Would you object to a change to the obsstore that banned creation of
> cycles?

This was supposedly the point of

http://www.selenic.com/hg/rev/a56038e6a3c9

Were you not running the tip?
Augie Fackler - Aug. 18, 2014, 11:06 p.m.
On Aug 18, 2014, at 7:05 PM, Matt Mackall <mpm@selenic.com> wrote:

> On Mon, 2014-08-18 at 18:31 -0400, Augie Fackler wrote:
>> On Fri, Aug 15, 2014 at 10:10:06AM -0700, Pierre-Yves David wrote:
>>> # HG changeset patch
>>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>>> # Date 1403625580 -3600
>>> #      Tue Jun 24 16:59:40 2014 +0100
>>> # Node ID b12294b39da84f2dad4d416161f5e48bb9966da3
>>> # Parent  8dda6f6ff564d8fe6ac7b8ce4c74eb9bfb5de14a
>>> revert: add a XXX about rename tracking
>> 
>> Queued. BTW when I queued this with 8dda as my working copy, 'hg
>> import --obsolete' gave me a self-obsoleting change again. It took
>> some fighting to get out of the bad state.
>> 
>> Would you object to a change to the obsstore that banned creation of
>> cycles?
> 
> This was supposedly the point of
> 
> http://www.selenic.com/hg/rev/a56038e6a3c9
> 
> Were you not running the tip?

Yup. Pierre-Yves already caught this on IRC. This machine must have gotten missed when I upgraded other machines. Sigh.

> 
> -- 
> Mathematics is the supreme nostalgia of our time.
> 
>
Pierre-Yves David - Aug. 18, 2014, 11:07 p.m.
On 08/18/2014 04:05 PM, Matt Mackall wrote:
> On Mon, 2014-08-18 at 18:31 -0400, Augie Fackler wrote:
>> On Fri, Aug 15, 2014 at 10:10:06AM -0700, Pierre-Yves David wrote:
>>> # HG changeset patch
>>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>>> # Date 1403625580 -3600
>>> #      Tue Jun 24 16:59:40 2014 +0100
>>> # Node ID b12294b39da84f2dad4d416161f5e48bb9966da3
>>> # Parent  8dda6f6ff564d8fe6ac7b8ce4c74eb9bfb5de14a
>>> revert: add a XXX about rename tracking
>>
>> Queued. BTW when I queued this with 8dda as my working copy, 'hg
>> import --obsolete' gave me a self-obsoleting change again. It took
>> some fighting to get out of the bad state.
>>
>> Would you object to a change to the obsstore that banned creation of
>> cycles?
>
> This was supposedly the point of
>
> http://www.selenic.com/hg/rev/a56038e6a3c9
>
> Were you not running the tip?

He was not. I got him to update both mercurial and evolve.

Patch

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -2422,10 +2422,11 @@  def revert(ui, repo, ctx, parents, *pats
 
         # if f is a rename, update `names` to also revert the source
         cwd = repo.getcwd()
         for f in dsadded:
             src = repo.dirstate.copied(f)
+            # XXX should we check for rename down to target node?
             if src and src not in names and repo.dirstate[src] == 'r':
                 dsremoved.add(src)
                 names[src] = (repo.pathto(src, cwd), True)
 
         ## computation of the action to performs on `names` content.