Patchwork [1,of,2,stable] tests: add test for rebasing merges with ancestors of the rebase destination

login
register
mail settings
Submitter Mads Kiilerich
Date Nov. 8, 2014, 1:27 a.m.
Message ID <8f689654b7d3d6b7c2b3.1415410047@localhost.localdomain>
Download mbox | patch
Permalink /patch/6648/
State Superseded
Commit 65f215ea3e8e3be7bd13ac4b8f3b40568d12ec66
Headers show

Comments

Mads Kiilerich - Nov. 8, 2014, 1:27 a.m.
# HG changeset patch
# User Mads Kiilerich <madski@unity3d.com>
# Date 1415409335 -3600
#      Sat Nov 08 02:15:35 2014 +0100
# Branch stable
# Node ID 8f689654b7d3d6b7c2b370cd5e38198b625e4fc6
# Parent  c35ffa4249cab47a1e089a30bc16fc65a0727f48
tests: add test for rebasing merges with ancestors of the rebase destination

This shows sub-optimal behaviour. The user get a merge prompt that is very hard
to explain.
Martin von Zweigbergk - Nov. 8, 2014, 5:36 a.m.
On Fri Nov 07 2014 at 5:28:28 PM Mads Kiilerich <mads@kiilerich.com> wrote:

> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1415409335 -3600
> #      Sat Nov 08 02:15:35 2014 +0100
> # Branch stable
> # Node ID 8f689654b7d3d6b7c2b370cd5e38198b625e4fc6
> # Parent  c35ffa4249cab47a1e089a30bc16fc65a0727f48
> tests: add test for rebasing merges with ancestors of the rebase
> destination
>
> This shows sub-optimal behaviour. The user get a merge prompt that is very
> hard
> to explain.
>

It seems more like you want the feature to work differently than it
currently does than a bug (I'm wondering if the "stable" flag is
appropriate). It's not obvious what the right behavior is. With the
suggestions below, it might be easier to tell what this is about.


> diff --git a/tests/test-rebase-newancestor.t b/tests/test-rebase-
> newancestor.t
> --- a/tests/test-rebase-newancestor.t
> +++ b/tests/test-rebase-newancestor.t
> @@ -53,3 +53,42 @@
>
>
>    $ cd ..
> +
> +
> +Test rebasing of merges with ancestors of the rebase destination - a
> situation
> +that often happens when trying to rebase instead of repeated merges.
> +
> +  $ hg init repo2
> +  $ cd repo2
> +  $ touch f
> +  $ hg ci -Aqm 'f'
> +  $ echo stuff > f
> +  $ hg ci -m 'f stuff'
> +  $ hg rm f
> +  $ hg ci -m 'remove f'
> +  $ hg up -qr0
> +  $ hg branch -q dev
> +  $ hg ci -qm 'dev'
> +  $ hg merge -q 1
> +  $ hg ci -m 'merge f stuff'
> +  $ hg merge -q 2
> +  $ hg ci -m 'merge remove f'
> +  $ touch devstuff
> +  $ hg ci -Aqm 'real dev stuff'
>

A graph log at this point would make the history clear. Now the reader has
to create that graph in their head.


> +  $ hg rebase -r 'branch(dev)' -d default
> +  remote changed f which local deleted
> +  use (c)hanged version or leave (d)eleted? c
> +  local changed f which remote deleted
> +  use (c)hanged version or (d)elete? c
>

The conflicts just seem like distraction. Would you be able to demonstrate
the same behavior without conflicts?


> +  saved backup bundle to $TESTTMP/repo2/.hg/strip-backup/*-backup.hg
> (glob)
> +  $ hg tglog
> +  @  4: 'real dev stuff'
> +  |
> +  o  3: 'merge f stuff'
> +  |
> +  o  2: 'remove f'
> +  |
> +  o  1: 'f stuff'
> +  |
> +  o  0: 'f'
> +
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
Mads Kiilerich - Nov. 8, 2014, 11:38 a.m.
On 11/08/2014 06:36 AM, Martin von Zweigbergk wrote:
>
>
> On Fri Nov 07 2014 at 5:28:28 PM Mads Kiilerich <mads@kiilerich.com 
> <mailto:mads@kiilerich.com>> wrote:
>
>     # HG changeset patch
>     # User Mads Kiilerich <madski@unity3d.com <mailto:madski@unity3d.com>>
>     # Date 1415409335 -3600
>     #      Sat Nov 08 02:15:35 2014 +0100
>     # Branch stable
>     # Node ID 8f689654b7d3d6b7c2b370cd5e38198b625e4fc6
>     # Parent  c35ffa4249cab47a1e089a30bc16fc65a0727f48
>     tests: add test for rebasing merges with ancestors of the rebase
>     destination
>
>     This shows sub-optimal behaviour. The user get a merge prompt that
>     is very hard
>     to explain.
>
>
> It seems more like you want the feature to work differently than it 
> currently does than a bug (I'm wondering if the "stable" flag is 
> appropriate). It's not obvious what the right behavior is. With the 
> suggestions below, it might be easier to tell what this is about.

The test shows behaviour that doesn't make sense and is wrong. That is a 
bug. A bug that in some cases would give wrong results when rebasing. I 
thus think a fix on stable is appropriate.

>
>
>     diff --git a/tests/test-rebase-newancestor.t
>     b/tests/test-rebase-newancestor.t
>     --- a/tests/test-rebase-newancestor.t
>     +++ b/tests/test-rebase-newancestor.t
>     @@ -53,3 +53,42 @@
>
>
>        $ cd ..
>     +
>     +
>     +Test rebasing of merges with ancestors of the rebase destination
>     - a situation
>     +that often happens when trying to rebase instead of repeated merges.
>     +
>     +  $ hg init repo2
>     +  $ cd repo2
>     +  $ touch f
>     +  $ hg ci -Aqm 'f'
>     +  $ echo stuff > f
>     +  $ hg ci -m 'f stuff'
>     +  $ hg rm f
>     +  $ hg ci -m 'remove f'
>     +  $ hg up -qr0
>     +  $ hg branch -q dev
>     +  $ hg ci -qm 'dev'
>     +  $ hg merge -q 1
>     +  $ hg ci -m 'merge f stuff'
>     +  $ hg merge -q 2
>     +  $ hg ci -m 'merge remove f'
>     +  $ touch devstuff
>     +  $ hg ci -Aqm 'real dev stuff'
>
>
> A graph log at this point would make the history clear. Now the reader 
> has to create that graph in their head.

I can add it. The graph is as big as the commands for creating the case 
and it doesn't look that nice. (I could create the test case in a more 
complicated way and make the graph looks nicer. Meh.)

>     +  $ hg rebase -r 'branch(dev)' -d default
>     +  remote changed f which local deleted
>     +  use (c)hanged version or leave (d)eleted? c
>     +  local changed f which remote deleted
>     +  use (c)hanged version or (d)elete? c
>
>
> The conflicts just seem like distraction. Would you be able to 
> demonstrate the same behavior without conflicts?

Look at the test case. f is never touched in the changes we rebase. 
There _is_ no conflict. Rebasing an ancestor merge is in general 
inherently nonsensical.

>     +  saved backup bundle to
>     $TESTTMP/repo2/.hg/strip-backup/*-backup.hg (glob)
>     +  $ hg tglog
>     +  @  4: 'real dev stuff'
>     +  |
>     +  o  3: 'merge f stuff'
>     +  |
>     +  o  2: 'remove f'
>     +  |
>     +  o  1: 'f stuff'
>     +  |
>     +  o  0: 'f'
>     +
>     _______________________________________________
>     Mercurial-devel mailing list
>     Mercurial-devel@selenic.com <mailto:Mercurial-devel@selenic.com>
>     http://selenic.com/mailman/listinfo/mercurial-devel
>
Pierre-Yves David - Nov. 8, 2014, 12:55 p.m.
On 11/08/2014 01:27 AM, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1415409335 -3600
> #      Sat Nov 08 02:15:35 2014 +0100
> # Branch stable
> # Node ID 8f689654b7d3d6b7c2b370cd5e38198b625e4fc6
> # Parent  c35ffa4249cab47a1e089a30bc16fc65a0727f48
> tests: add test for rebasing merges with ancestors of the rebase destination

More thinking and discussion about this convinced me that this change is 
bad and will introduce mishaviour (if some important change are 
introduced during a merge for eg we'll silently drop them).

I'm dropping this series and waiting for Mads to provide more extensive 
explanation of what are the problematic case, why is rebase behaving sub 
optimally and how we could fixes it properly.
Martin von Zweigbergk - Nov. 8, 2014, 6:11 p.m.
On Nov 8, 2014 3:38 AM, "Mads Kiilerich" <mads@kiilerich.com> wrote:
>
> On 11/08/2014 06:36 AM, Martin von Zweigbergk wrote:
>>
>>
>>
>> On Fri Nov 07 2014 at 5:28:28 PM Mads Kiilerich <mads@kiilerich.com>
wrote:
>>>
>>> # HG changeset patch
>>> # User Mads Kiilerich <madski@unity3d.com>
>>> # Date 1415409335 -3600
>>> #      Sat Nov 08 02:15:35 2014 +0100
>>> # Branch stable
>>> # Node ID 8f689654b7d3d6b7c2b370cd5e38198b625e4fc6
>>> # Parent  c35ffa4249cab47a1e089a30bc16fc65a0727f48
>>> tests: add test for rebasing merges with ancestors of the rebase
destination
>>>
>>> This shows sub-optimal behaviour. The user get a merge prompt that is
very hard
>>> to explain.
>>
>>
>> It seems more like you want the feature to work differently than it
currently does than a bug (I'm wondering if the "stable" flag is
appropriate). It's not obvious what the right behavior is. With the
suggestions below, it might be easier to tell what this is about.
>
>
> The test shows behaviour that doesn't make sense and is wrong. That is a
bug. A bug that in some cases would give wrong results when rebasing. I
thus think a fix on stable is appropriate.
>
>
>>
>>>
>>> diff --git a/tests/test-rebase-newancestor.t
b/tests/test-rebase-newancestor.t
>>> --- a/tests/test-rebase-newancestor.t
>>> +++ b/tests/test-rebase-newancestor.t
>>> @@ -53,3 +53,42 @@
>>>
>>>
>>>    $ cd ..
>>> +
>>> +
>>> +Test rebasing of merges with ancestors of the rebase destination - a
situation
>>> +that often happens when trying to rebase instead of repeated merges.
>>> +
>>> +  $ hg init repo2
>>> +  $ cd repo2
>>> +  $ touch f
>>> +  $ hg ci -Aqm 'f'
>>> +  $ echo stuff > f
>>> +  $ hg ci -m 'f stuff'
>>> +  $ hg rm f
>>> +  $ hg ci -m 'remove f'
>>> +  $ hg up -qr0
>>> +  $ hg branch -q dev
>>> +  $ hg ci -qm 'dev'
>>> +  $ hg merge -q 1
>>> +  $ hg ci -m 'merge f stuff'
>>> +  $ hg merge -q 2
>>> +  $ hg ci -m 'merge remove f'
>>> +  $ touch devstuff
>>> +  $ hg ci -Aqm 'real dev stuff'
>>
>>
>> A graph log at this point would make the history clear. Now the reader
has to create that graph in their head.
>
>
> I can add it. The graph is as big as the commands for creating the case
and it doesn't look that nice. (I could create the test case in a more
complicated way and make the graph looks nicer. Meh.)

How about a graph in the commit message at least then? Like this before?

---0---1---2
    \   \
     3---4---5

And whatever is the current state after rebase and what you think should be
the right state.

>
>
>>
>>>
>>> +  $ hg rebase -r 'branch(dev)' -d default
>>> +  remote changed f which local deleted
>>> +  use (c)hanged version or leave (d)eleted? c
>>> +  local changed f which remote deleted
>>> +  use (c)hanged version or (d)elete? c
>>
>>
>> The conflicts just seem like distraction. Would you be able to
demonstrate the same behavior without conflicts?
>
>
> Look at the test case. f is never touched in the changes we rebase. There
_is_ no conflict.

So what does "remote changed f which local deleted" mean if not that there
is a conflict?

> Rebasing an ancestor merge is in general inherently nonsensical.

In general yes, but for so-called evil merges, there is information that
gets lost (as Pierre-Yves also pointed out). It's a hard problem, though.

>
>
>>
>>>
>>> +  saved backup bundle to $TESTTMP/repo2/.hg/strip-backup/*-backup.hg
(glob)
>>> +  $ hg tglog
>>> +  @  4: 'real dev stuff'
>>> +  |
>>> +  o  3: 'merge f stuff'
>>> +  |
>>> +  o  2: 'remove f'
>>> +  |
>>> +  o  1: 'f stuff'
>>> +  |
>>> +  o  0: 'f'
>>> +
>>> _______________________________________________
>>> Mercurial-devel mailing list
>>> Mercurial-devel@selenic.com
>>> http://selenic.com/mailman/listinfo/mercurial-devel
>
>
Mads Kiilerich - Nov. 8, 2014, 9:42 p.m.
On 11/08/2014 07:11 PM, Martin von Zweigbergk wrote:
> >>> +  $ hg rebase -r 'branch(dev)' -d default
>
> >>> +  remote changed f which local deleted
> >>> +  use (c)hanged version or leave (d)eleted? c
> >>> +  local changed f which remote deleted
> >>> +  use (c)hanged version or (d)elete? c
> >>
> >>
> >> The conflicts just seem like distraction. Would you be able to 
> demonstrate the same behavior without conflicts?
> >
> >
> > Look at the test case. f is never touched in the changes we rebase. 
> There _is_ no conflict.
>
> So what does "remote changed f which local deleted" mean if not that 
> there is a conflict?
>

It is garbage in garbage out. It is using an invalid merge ancestor and 
thus getting a result that doesn't make sense. Also, there _is_ no 
correct ancestor. The problem is thus not that it is using a wrong 
ancestor but that it is trying to merge.

> > Rebasing an ancestor merge is in general inherently nonsensical.
>
> In general yes, but for so-called evil merges, there is information 
> that gets lost (as Pierre-Yves also pointed out).
>

That information will be lost anyway if the user has to dig it out among 
a lot of misleading and wrong information.

/Mads
Pierre-Yves David - Nov. 8, 2014, 9:57 p.m.
Simple baseline:

   Merges may contains valuable information, rebase is never going to 
drop them.

You can search for improvement inside theses rule. I'm going to reject 
any solution that drop merges because they are merge.

In my opinion, the search for this solution starts with a proper wording 
of the problem you face and the various element involved. I'm not going 
to trust your solutions until I trust your analysis.

Cheers,

Patch

diff --git a/tests/test-rebase-newancestor.t b/tests/test-rebase-newancestor.t
--- a/tests/test-rebase-newancestor.t
+++ b/tests/test-rebase-newancestor.t
@@ -53,3 +53,42 @@ 
   
 
   $ cd ..
+
+
+Test rebasing of merges with ancestors of the rebase destination - a situation
+that often happens when trying to rebase instead of repeated merges.
+
+  $ hg init repo2
+  $ cd repo2
+  $ touch f
+  $ hg ci -Aqm 'f'
+  $ echo stuff > f
+  $ hg ci -m 'f stuff'
+  $ hg rm f
+  $ hg ci -m 'remove f'
+  $ hg up -qr0
+  $ hg branch -q dev
+  $ hg ci -qm 'dev'
+  $ hg merge -q 1
+  $ hg ci -m 'merge f stuff'
+  $ hg merge -q 2
+  $ hg ci -m 'merge remove f'
+  $ touch devstuff
+  $ hg ci -Aqm 'real dev stuff'
+  $ hg rebase -r 'branch(dev)' -d default
+  remote changed f which local deleted
+  use (c)hanged version or leave (d)eleted? c
+  local changed f which remote deleted
+  use (c)hanged version or (d)elete? c
+  saved backup bundle to $TESTTMP/repo2/.hg/strip-backup/*-backup.hg (glob)
+  $ hg tglog
+  @  4: 'real dev stuff'
+  |
+  o  3: 'merge f stuff'
+  |
+  o  2: 'remove f'
+  |
+  o  1: 'f stuff'
+  |
+  o  0: 'f'
+