Patchwork [STABLE] revert: properly revert to ancestor of p2 during merge

login
register
mail settings
Submitter Pierre-Yves David
Date Feb. 23, 2016, 12:02 p.m.
Message ID <aa94f42b742e958bd9c3.1456228949@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/13307/
State Accepted
Delegated to: Martin von Zweigbergk
Headers show

Comments

Pierre-Yves David - Feb. 23, 2016, 12:02 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1456224107 -3600
#      Tue Feb 23 11:41:47 2016 +0100
# Branch stable
# Node ID aa94f42b742e958bd9c3f2f7a82722a7058aee76
# Parent  1bcb4f34b9f91a2e330966182f691664fbada1bc
# Available At http://hg.netv6.net/marmoute-wip/mercurial/
#              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r aa94f42b742e
revert: properly revert to ancestor of p2 during merge

During merge, added (from one perspective) file can be reported as "modified".
To work around that, revert was testing if modified file where present in the
parent manifest and marking them as "added" in this case. However, we should be
checking against the target revision manifest instead. Otherwise see file as
"newly added" even if they exist in the target revision.

That revert behavior regressed in 06fbd9518bc5.

Note: in this patch we can notice that we are building a set out of a manifest,
breaking effort for manifest laziness introduced since this code was written.
Fixing it is another adventure.
Pierre-Yves David - Feb. 23, 2016, 12:16 p.m.
On 02/23/2016 01:02 PM, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1456224107 -3600
> #      Tue Feb 23 11:41:47 2016 +0100
> # Branch stable
> # Node ID aa94f42b742e958bd9c3f2f7a82722a7058aee76
> # Parent  1bcb4f34b9f91a2e330966182f691664fbada1bc
> # Available At http://hg.netv6.net/marmoute-wip/mercurial/
> #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r aa94f42b742e
> revert: properly revert to ancestor of p2 during merge

I forgot to add (issue5052) on that first line.
Martin von Zweigbergk - Feb. 23, 2016, 6:04 p.m.
On Tue, Feb 23, 2016 at 4:02 AM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1456224107 -3600
> #      Tue Feb 23 11:41:47 2016 +0100
> # Branch stable
> # Node ID aa94f42b742e958bd9c3f2f7a82722a7058aee76
> # Parent  1bcb4f34b9f91a2e330966182f691664fbada1bc
> # Available At http://hg.netv6.net/marmoute-wip/mercurial/
> #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r aa94f42b742e
> revert: properly revert to ancestor of p2 during merge
>
> During merge, added (from one perspective) file can be reported as "modified".
> To work around that, revert was testing if modified file where present in the
> parent manifest and marking them as "added" in this case. However, we should be
> checking against the target revision manifest instead. Otherwise see file as
> "newly added" even if they exist in the target revision.
>
> That revert behavior regressed in 06fbd9518bc5.
>
> Note: in this patch we can notice that we are building a set out of a manifest,
> breaking effort for manifest laziness introduced since this code was written.
> Fixing it is another adventure.
>
> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -2844,14 +2844,10 @@ def revert(ui, repo, ctx, parents, *pats
>      node = ctx.node()
>
>      mf = ctx.manifest()
>      if node == p2:
>          parent = p2
> -    if node == parent:
> -        pmf = mf
> -    else:
> -        pmf = None
>
>      # need all matching names in dirstate and manifest of target rev,
>      # so have to walk both. do not print errors if files exist in one
>      # but not other. in both cases, filesets should be evaluated against
>      # workingctx to get consistent result (issue4497). this means 'set:**'
> @@ -2962,15 +2958,11 @@ def revert(ui, repo, ctx, parents, *pats
>              dsadded = added
>
>          # in case of merge, files that are actually added can be reported as
>          # modified, we need to post process the result
>          if p2 != nullid:
> -            if pmf is None:
> -                # only need parent manifest in the merge case,
> -                # so do not read by default
> -                pmf = repo[parent].manifest()
> -            mergeadd = dsmodified - set(pmf)
> +            mergeadd = dsmodified - set(mf)


I have replaced "set(mf)" by "smf", which was already defined. Does
that mean I should also drop the part about "breaking effort for
manifest laziness" in the commit message? What was that effort? Could
you point to some commits?

>              dsadded |= mergeadd
>              dsmodified -= mergeadd
>
>          # if f is a rename, update `names` to also revert the source
>          cwd = repo.getcwd()
> diff --git a/tests/test-revert.t b/tests/test-revert.t
> --- a/tests/test-revert.t
> +++ b/tests/test-revert.t
> @@ -1074,5 +1074,74 @@ check resulting directory against the --
>
>    $ python ../dircontent.py > ../content-base-explicit.txt
>    $ cd ..
>    $ diff -U 0 -- content-base-all.txt content-base-explicit.txt | grep _
>    [1]
> +
> +Revert to an ancestor of P2 during a merge (issue5052)
> +-----------------------------------------------------
> +
> +(prepare the repository)
> +
> +  $ hg init issue5052
> +  $ cd issue5052
> +  $ echo '.\.orig' > .hgignore
> +  $ echo 0 > root
> +  $ hg ci -qAm C0
> +  $ echo 0 > A
> +  $ hg ci -qAm C1
> +  $ echo 1 >> A
> +  $ hg ci -qm C2
> +  $ hg up -q 0
> +  $ echo 1 > B
> +  $ hg ci -qAm C3
> +  $ hg status --rev 'ancestor(.,2)' --rev 2
> +  A A
> +  $ hg log -G -T '{rev} ({files})\n'
> +  @  3 (B)
> +  |
> +  | o  2 (A)
> +  | |
> +  | o  1 (A)
> +  |/
> +  o  0 (.hgignore root)
> +
> +
> +actual tests: reverting to something else than a merge parent
> +
> +  $ hg merge
> +  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  (branch merge, don't forget to commit)
> +
> +  $ hg status --rev 'p1()'
> +  M A
> +  $ hg status --rev 'p2()'
> +  A B
> +  $ hg status --rev '1'
> +  M A
> +  A B
> +  $ hg revert --rev 1 --all
> +  reverting A
> +  removing B
> +  $ hg status --rev 1
> +
> +From the other parents
> +
> +  $ hg up -C 'p2()'
> +  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  $ hg merge
> +  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  (branch merge, don't forget to commit)
> +
> +  $ hg status --rev 'p1()'
> +  M B
> +  $ hg status --rev 'p2()'
> +  A A
> +  $ hg status --rev '1'
> +  M A
> +  A B
> +  $ hg revert --rev 1 --all
> +  reverting A
> +  removing B
> +  $ hg status --rev 1
> +
> +  $ cd ..
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Pierre-Yves David - Feb. 23, 2016, 6:17 p.m.
On 02/23/2016 07:04 PM, Martin von Zweigbergk wrote:
> On Tue, Feb 23, 2016 at 4:02 AM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org> wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>> # Date 1456224107 -3600
>> #      Tue Feb 23 11:41:47 2016 +0100
>> # Branch stable
>> # Node ID aa94f42b742e958bd9c3f2f7a82722a7058aee76
>> # Parent  1bcb4f34b9f91a2e330966182f691664fbada1bc
>> # Available At http://hg.netv6.net/marmoute-wip/mercurial/
>> #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r aa94f42b742e
>> revert: properly revert to ancestor of p2 during merge
>>
>> During merge, added (from one perspective) file can be reported as "modified".
>> To work around that, revert was testing if modified file where present in the
>> parent manifest and marking them as "added" in this case. However, we should be
>> checking against the target revision manifest instead. Otherwise see file as
>> "newly added" even if they exist in the target revision.
>>
>> That revert behavior regressed in 06fbd9518bc5.
>>
>> Note: in this patch we can notice that we are building a set out of a manifest,
>> breaking effort for manifest laziness introduced since this code was written.
>> Fixing it is another adventure.
>>
>> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
>> --- a/mercurial/cmdutil.py
>> +++ b/mercurial/cmdutil.py
>> @@ -2844,14 +2844,10 @@ def revert(ui, repo, ctx, parents, *pats
>>       node = ctx.node()
>>
>>       mf = ctx.manifest()
>>       if node == p2:
>>           parent = p2
>> -    if node == parent:
>> -        pmf = mf
>> -    else:
>> -        pmf = None
>>
>>       # need all matching names in dirstate and manifest of target rev,
>>       # so have to walk both. do not print errors if files exist in one
>>       # but not other. in both cases, filesets should be evaluated against
>>       # workingctx to get consistent result (issue4497). this means 'set:**'
>> @@ -2962,15 +2958,11 @@ def revert(ui, repo, ctx, parents, *pats
>>               dsadded = added
>>
>>           # in case of merge, files that are actually added can be reported as
>>           # modified, we need to post process the result
>>           if p2 != nullid:
>> -            if pmf is None:
>> -                # only need parent manifest in the merge case,
>> -                # so do not read by default
>> -                pmf = repo[parent].manifest()
>> -            mergeadd = dsmodified - set(pmf)
>> +            mergeadd = dsmodified - set(mf)
>
>
> I have replaced "set(mf)" by "smf", which was already defined.

Ha good point, thanks

> Does
> that mean I should also drop the part about "breaking effort for
> manifest laziness" in the commit message? What was that effort? Could
> you point to some commits?

You can drop it, we are not worse that what smf is doing.
I'm speaking at the whole durin42/google effort to have manifest related 
operation lazy.
Martin von Zweigbergk - Feb. 23, 2016, 6:35 p.m.
On Tue, Feb 23, 2016 at 10:17 AM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
>
>
> On 02/23/2016 07:04 PM, Martin von Zweigbergk wrote:
>>
>> On Tue, Feb 23, 2016 at 4:02 AM, Pierre-Yves David
>> <pierre-yves.david@ens-lyon.org> wrote:
>>>
>>> # HG changeset patch
>>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>>> # Date 1456224107 -3600
>>> #      Tue Feb 23 11:41:47 2016 +0100
>>> # Branch stable
>>> # Node ID aa94f42b742e958bd9c3f2f7a82722a7058aee76
>>> # Parent  1bcb4f34b9f91a2e330966182f691664fbada1bc
>>> # Available At http://hg.netv6.net/marmoute-wip/mercurial/
>>> #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r
>>> aa94f42b742e
>>> revert: properly revert to ancestor of p2 during merge
>>>
>>> During merge, added (from one perspective) file can be reported as
>>> "modified".
>>> To work around that, revert was testing if modified file where present in
>>> the
>>> parent manifest and marking them as "added" in this case. However, we
>>> should be
>>> checking against the target revision manifest instead. Otherwise see file
>>> as
>>> "newly added" even if they exist in the target revision.
>>>
>>> That revert behavior regressed in 06fbd9518bc5.
>>>
>>> Note: in this patch we can notice that we are building a set out of a
>>> manifest,
>>> breaking effort for manifest laziness introduced since this code was
>>> written.
>>> Fixing it is another adventure.
>>>
>>> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
>>> --- a/mercurial/cmdutil.py
>>> +++ b/mercurial/cmdutil.py
>>> @@ -2844,14 +2844,10 @@ def revert(ui, repo, ctx, parents, *pats
>>>       node = ctx.node()
>>>
>>>       mf = ctx.manifest()
>>>       if node == p2:
>>>           parent = p2
>>> -    if node == parent:
>>> -        pmf = mf
>>> -    else:
>>> -        pmf = None
>>>
>>>       # need all matching names in dirstate and manifest of target rev,
>>>       # so have to walk both. do not print errors if files exist in one
>>>       # but not other. in both cases, filesets should be evaluated
>>> against
>>>       # workingctx to get consistent result (issue4497). this means
>>> 'set:**'
>>> @@ -2962,15 +2958,11 @@ def revert(ui, repo, ctx, parents, *pats
>>>               dsadded = added
>>>
>>>           # in case of merge, files that are actually added can be
>>> reported as
>>>           # modified, we need to post process the result
>>>           if p2 != nullid:
>>> -            if pmf is None:
>>> -                # only need parent manifest in the merge case,
>>> -                # so do not read by default
>>> -                pmf = repo[parent].manifest()
>>> -            mergeadd = dsmodified - set(pmf)
>>> +            mergeadd = dsmodified - set(mf)
>>
>>
>>
>> I have replaced "set(mf)" by "smf", which was already defined.
>
>
> Ha good point, thanks
>
>> Does
>> that mean I should also drop the part about "breaking effort for
>> manifest laziness" in the commit message? What was that effort? Could
>> you point to some commits?
>
>
> You can drop it, we are not worse that what smf is doing.

Done. Pushed to the clowncopter.

> I'm speaking at the whole durin42/google effort to have manifest related
> operation lazy.

I thought you might be referring to my treemanifest work, but it
turned out I had not made any relevant changes to revert. However, I
found some old patches in my repo that I should take another look at
:-)

Patch

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -2844,14 +2844,10 @@  def revert(ui, repo, ctx, parents, *pats
     node = ctx.node()
 
     mf = ctx.manifest()
     if node == p2:
         parent = p2
-    if node == parent:
-        pmf = mf
-    else:
-        pmf = None
 
     # need all matching names in dirstate and manifest of target rev,
     # so have to walk both. do not print errors if files exist in one
     # but not other. in both cases, filesets should be evaluated against
     # workingctx to get consistent result (issue4497). this means 'set:**'
@@ -2962,15 +2958,11 @@  def revert(ui, repo, ctx, parents, *pats
             dsadded = added
 
         # in case of merge, files that are actually added can be reported as
         # modified, we need to post process the result
         if p2 != nullid:
-            if pmf is None:
-                # only need parent manifest in the merge case,
-                # so do not read by default
-                pmf = repo[parent].manifest()
-            mergeadd = dsmodified - set(pmf)
+            mergeadd = dsmodified - set(mf)
             dsadded |= mergeadd
             dsmodified -= mergeadd
 
         # if f is a rename, update `names` to also revert the source
         cwd = repo.getcwd()
diff --git a/tests/test-revert.t b/tests/test-revert.t
--- a/tests/test-revert.t
+++ b/tests/test-revert.t
@@ -1074,5 +1074,74 @@  check resulting directory against the --
 
   $ python ../dircontent.py > ../content-base-explicit.txt
   $ cd ..
   $ diff -U 0 -- content-base-all.txt content-base-explicit.txt | grep _
   [1]
+
+Revert to an ancestor of P2 during a merge (issue5052)
+-----------------------------------------------------
+
+(prepare the repository)
+
+  $ hg init issue5052
+  $ cd issue5052
+  $ echo '.\.orig' > .hgignore
+  $ echo 0 > root
+  $ hg ci -qAm C0
+  $ echo 0 > A
+  $ hg ci -qAm C1
+  $ echo 1 >> A
+  $ hg ci -qm C2
+  $ hg up -q 0
+  $ echo 1 > B
+  $ hg ci -qAm C3
+  $ hg status --rev 'ancestor(.,2)' --rev 2
+  A A
+  $ hg log -G -T '{rev} ({files})\n'
+  @  3 (B)
+  |
+  | o  2 (A)
+  | |
+  | o  1 (A)
+  |/
+  o  0 (.hgignore root)
+  
+
+actual tests: reverting to something else than a merge parent
+
+  $ hg merge
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  (branch merge, don't forget to commit)
+
+  $ hg status --rev 'p1()'
+  M A
+  $ hg status --rev 'p2()'
+  A B
+  $ hg status --rev '1'
+  M A
+  A B
+  $ hg revert --rev 1 --all
+  reverting A
+  removing B
+  $ hg status --rev 1
+
+From the other parents
+
+  $ hg up -C 'p2()'
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  $ hg merge
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  (branch merge, don't forget to commit)
+
+  $ hg status --rev 'p1()'
+  M B
+  $ hg status --rev 'p2()'
+  A A
+  $ hg status --rev '1'
+  M A
+  A B
+  $ hg revert --rev 1 --all
+  reverting A
+  removing B
+  $ hg status --rev 1
+
+  $ cd ..