Patchwork [3,of,5] merge: synthesize L+O records in mergestate for conflictfree merges

login
register
mail settings
Submitter timeless@mozdev.org
Date March 17, 2016, 3:46 p.m.
Message ID <90b56e5e614a0696ded4.1458229610@waste.org>
Download mbox | patch
Permalink /patch/13921/
State Deferred
Headers show

Comments

timeless@mozdev.org - March 17, 2016, 3:46 p.m.
# HG changeset patch
# User timeless <timeless@mozdev.org>
# Date 1458226168 0
#      Thu Mar 17 14:49:28 2016 +0000
# Node ID 90b56e5e614a0696ded4e75349317b55b9c30533
# Parent  18892389001b3c991524cf04bbb004a7a1a15188
merge: synthesize L+O records in mergestate for conflictfree merges
timeless - March 17, 2016, 3:59 p.m.
It's possible to write a test for this using python, but it doesn't
seem worth it.
Without this change, ~2 of the standard tests would fail for 5 of 5 of
this series:

+    File "mercurial/commands.py", line 6548, in summary
+      if p == ms.localctx:
+    File "mercurial/util.py", line 724, in __get__
+      result = self.func(obj)
+    File "mercurial/merge.py", line 308, in localctx
+      raise RuntimeError("localctx accessed but self._local isn't set")
+  RuntimeError: localctx accessed but self._local isn't set
+  [1]

Failed test-revert.t: output changed
Failed test-backout.t: output changed


On Thu, Mar 17, 2016 at 11:46 AM, timeless <timeless@mozdev.org> wrote:
> # HG changeset patch
> # User timeless <timeless@mozdev.org>
> # Date 1458226168 0
> #      Thu Mar 17 14:49:28 2016 +0000
> # Node ID 90b56e5e614a0696ded4e75349317b55b9c30533
> # Parent  18892389001b3c991524cf04bbb004a7a1a15188
> merge: synthesize L+O records in mergestate for conflictfree merges
>
> diff --git a/mercurial/merge.py b/mercurial/merge.py
> --- a/mercurial/merge.py
> +++ b/mercurial/merge.py
> @@ -189,13 +189,21 @@
>          returns list of record [(TYPE, data), ...]"""
>          v1records = self._readrecordsv1()
>          v2records = self._readrecordsv2()
> +        parents = self._repo[None].parents()
> +        if len(parents) == 2 and not v1records and not v2records:
> +            # if we have two parents, we have a merge,
> +            # but if there were no conflicts, there will not be a mergestate.
> +            # localctx and otherctx expect L and O records to be present, so
> +            # synthesize them.
> +            p1, p2 = parents
> +            return [('L', hex(p1.node())), ('O', hex(p2.node()))]
>          if self._v1v2match(v1records, v2records):
>              return v2records
>          else:
>              # v1 file is newer than v2 file, use it
>              # we have to infer the "other" changeset of the merge
>              # we cannot do better than that with v1 of the format
> -            mctx = self._repo[None].parents()[-1]
> +            mctx = parents[-1]
>              v1records.append(('O', mctx.hex()))
>              # add place holder "other" file node information
>              # nobody is using it yet so we do no need to fetch the data
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Simon Farnsworth - March 18, 2016, 4:01 p.m.
On 17/03/2016, 08:46, "Mercurial-devel on behalf of timeless" <mercurial-devel-bounces@mercurial-scm.org on behalf of timeless@mozdev.org> wrote:



># HG changeset patch

># User timeless <timeless@mozdev.org>

># Date 1458226168 0

>#      Thu Mar 17 14:49:28 2016 +0000

># Node ID 90b56e5e614a0696ded4e75349317b55b9c30533

># Parent  18892389001b3c991524cf04bbb004a7a1a15188

>merge: synthesize L+O records in mergestate for conflictfree merges


Why do we need this? What use is localctx and otherctx if you're not going to attempt a fresh merge (I have some thoughts, but...).

Would we be better off with (local|other)ctx succeeding rather than throwing an exception if there's no record for the merge point?
>

>diff --git a/mercurial/merge.py b/mercurial/merge.py

>--- a/mercurial/merge.py

>+++ b/mercurial/merge.py

>@@ -189,13 +189,21 @@

>         returns list of record [(TYPE, data), ...]"""

>         v1records = self._readrecordsv1()

>         v2records = self._readrecordsv2()

>+        parents = self._repo[None].parents()

>+        if len(parents) == 2 and not v1records and not v2records:

>+            # if we have two parents, we have a merge,

>+            # but if there were no conflicts, there will not be a mergestate.

>+            # localctx and otherctx expect L and O records to be present, so

>+            # synthesize them.

>+            p1, p2 = parents

>+            return [('L', hex(p1.node())), ('O', hex(p2.node()))]

>         if self._v1v2match(v1records, v2records):

>             return v2records

>         else:

>             # v1 file is newer than v2 file, use it

>             # we have to infer the "other" changeset of the merge

>             # we cannot do better than that with v1 of the format

>-            mctx = self._repo[None].parents()[-1]

>+            mctx = parents[-1]

>             v1records.append(('O', mctx.hex()))

>             # add place holder "other" file node information

>             # nobody is using it yet so we do no need to fetch the data

>_______________________________________________

>Mercurial-devel mailing list

>Mercurial-devel@mercurial-scm.org

>https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_mailman_listinfo_mercurial-2Ddevel&d=CwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=mEgSWILcY4c4W3zjApBQLA&m=bTYiH16NgzwNQ_W3jCgf3iVQRprWPTzcCMkv-cDa8yI&s=RwKOhGBbGGnZkvb2qqOLfMqAm4_TQ37vU2D3ts0KqD8&e=
timeless - March 18, 2016, 5:19 p.m.
Simon Farnsworth <simonfar@fb.com> wrote:
> Why do we need this? What use is localctx and otherctx if you're not going to attempt a fresh merge (I have some thoughts, but...).

It provides an easier way for my hg summary patch to calculate the
answer for what is local/what is other.

> Would we be better off with (local|other)ctx succeeding rather than throwing an exception if there's no record for the merge point?

That would also work.

There are two parents, we know there are two parents. Being forced to
look in different places is annoying. Having a localctx/otherctx only
some of the time is nonsensical. Just because there aren't any
conflicts doesn't mean I shouldn't be able to look at them...
Simon Farnsworth - March 18, 2016, 5:39 p.m.
On 18/03/2016, 10:19, "timeless.bmo1@gmail.com on behalf of timeless" <timeless.bmo1@gmail.com on behalf of timeless@gmail.com> wrote:



>Simon Farnsworth <simonfar@fb.com> wrote:

>> Why do we need this? What use is localctx and otherctx if you're not going to attempt a fresh merge (I have some thoughts, but...).

>

>It provides an easier way for my hg summary patch to calculate the

>answer for what is local/what is other.

>

>> Would we be better off with (local|other)ctx succeeding rather than throwing an exception if there's no record for the merge point?

>

>That would also work.

>

>There are two parents, we know there are two parents. Being forced to

>look in different places is annoying. Having a localctx/otherctx only

>some of the time is nonsensical. Just because there aren't any

>conflicts doesn't mean I shouldn't be able to look at them...


My gut feeling is that otherctx and localctx should always succeed using the information we have in memory if there's no mergestate, rather than synthesising records; I dislike synthetic records because of the risk of confusion between records we've synthesised, and records from disk. I'm willing to accept it when we're reading an old format file, and synthesising new format records, but otherwise it makes me feel bad.

Simon

Patch

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -189,13 +189,21 @@ 
         returns list of record [(TYPE, data), ...]"""
         v1records = self._readrecordsv1()
         v2records = self._readrecordsv2()
+        parents = self._repo[None].parents()
+        if len(parents) == 2 and not v1records and not v2records:
+            # if we have two parents, we have a merge,
+            # but if there were no conflicts, there will not be a mergestate.
+            # localctx and otherctx expect L and O records to be present, so
+            # synthesize them.
+            p1, p2 = parents
+            return [('L', hex(p1.node())), ('O', hex(p2.node()))]
         if self._v1v2match(v1records, v2records):
             return v2records
         else:
             # v1 file is newer than v2 file, use it
             # we have to infer the "other" changeset of the merge
             # we cannot do better than that with v1 of the format
-            mctx = self._repo[None].parents()[-1]
+            mctx = parents[-1]
             v1records.append(('O', mctx.hex()))
             # add place holder "other" file node information
             # nobody is using it yet so we do no need to fetch the data