Patchwork [4,of,5] merge.mergestate: store whether the merge is a branch merge

login
register
mail settings
Submitter Siddharth Agarwal
Date Nov. 3, 2015, 6:23 a.m.
Message ID <61c764167bb79d23f679.1446531813@dev6666.prn1.facebook.com>
Download mbox | patch
Permalink /patch/11271/
State Changes Requested
Headers show

Comments

Siddharth Agarwal - Nov. 3, 2015, 6:23 a.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1446520734 28800
#      Mon Nov 02 19:18:54 2015 -0800
# Node ID 61c764167bb79d23f6798a2d8352be38a5833770
# Parent  829d0c9e0591c5a90e64da8396575c536bcf9805
merge.mergestate: store whether the merge is a branch merge

This turns out to not actually be explicitly stored anywhere, and I couldn't figure out a way to infer it from the state that is there.

This is going to be useful for change/delete conflicts because depending on the
type of conflict, we might need to add or remove the file from the dirstate --
but only if this isn't a branch merge.

This is an optional (lowercase) parameter because this isn't actually used
unless there's a change/delete conflict. That will come in future patches, and
those will indeed use a separate uppercase character.
Martin von Zweigbergk - Nov. 4, 2015, 1:19 a.m.
On Mon, Nov 2, 2015 at 10:27 PM Siddharth Agarwal <sid0@fb.com> wrote:

> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1446520734 28800
> #      Mon Nov 02 19:18:54 2015 -0800
> # Node ID 61c764167bb79d23f6798a2d8352be38a5833770
> # Parent  829d0c9e0591c5a90e64da8396575c536bcf9805
> merge.mergestate: store whether the merge is a branch merge
>
> This turns out to not actually be explicitly stored anywhere, and I
> couldn't figure out a way to infer it from the state that is there.
>
> This is going to be useful for change/delete conflicts because depending
> on the
> type of conflict, we might need to add or remove the file from the
> dirstate --
> but only if this isn't a branch merge.
>
> This is an optional (lowercase) parameter because this isn't actually used
> unless there's a change/delete conflict. That will come in future patches,
> and
> those will indeed use a separate uppercase character.
>

I instead used a 'T' field to indicate the type of conflict, which could be
'm', 'cd' or 'dc'. It's been too long for me to understand what the pros
and cons of that would be versus your approach of storing the branch merge
flag. What do you think?

Here is my attempt, rebased part of the way to the current @:
http://42.netv6.net/martinvonz-wip/mercurial/log?rev=197a4ea5f82d%3A%3A89aa1f9405a5.
I don't even remember if there was some flaw in it or why I didn't send it
for review back then. It might be that the problems are now fixed by the
patches dealing with .hgsubstate that I saw a while back.


>
> diff --git a/mercurial/merge.py b/mercurial/merge.py
> --- a/mercurial/merge.py
> +++ b/mercurial/merge.py
> @@ -60,6 +60,7 @@ class mergestate(object):
>
>      L: the node of the "local" part of the merge (hexified version)
>      O: the node of the "other" part of the merge (hexified version)
> +    b: whether this is a branch merge
>      F: a file to be merged entry
>      D: a file that the external merge driver will merge internally
>         (experimental)
> @@ -80,7 +81,7 @@ class mergestate(object):
>          self._dirty = False
>          self._read()
>
> -    def reset(self, node=None, other=None):
> +    def reset(self, node=None, other=None, branchmerge=False):
>          self._state = {}
>          self._local = None
>          self._other = None
> @@ -89,6 +90,7 @@ class mergestate(object):
>          if node:
>              self._local = node
>              self._other = other
> +        self._branchmerge = branchmerge
>          self._readmergedriver = None
>          if self.mergedriver:
>              self._mdstate = 's'
> @@ -108,6 +110,7 @@ class mergestate(object):
>          self._other = None
>          if 'otherctx' in vars(self):
>              del self.otherctx
> +        self._branchmerge = False
>          self._readmergedriver = None
>          self._mdstate = 's'
>          records = self._readrecords()
> @@ -116,6 +119,8 @@ class mergestate(object):
>                  self._local = bin(record)
>              elif rtype == 'O':
>                  self._other = bin(record)
> +            elif rtype == 'b':
> +                self._branchmerge = bool(int(record))
>              elif rtype == 'm':
>                  bits = record.split('\0', 1)
>                  mdstate = bits[1]
> @@ -269,6 +274,7 @@ class mergestate(object):
>              records = []
>              records.append(('L', hex(self._local)))
>              records.append(('O', hex(self._other)))
> +            records.append(('b', str(int(self._branchmerge))))
>              if self.mergedriver:
>                  records.append(('m', '\0'.join([
>                      self.mergedriver, self._mdstate])))
> @@ -837,7 +843,7 @@ def applyupdates(repo, actions, wctx, mc
>
>      updated, merged, removed, unresolved = 0, 0, 0, 0
>      ms = mergestate(repo)
> -    ms.reset(wctx.p1().node(), mctx.node())
> +    ms.reset(wctx.p1().node(), mctx.node(), branchmerge)
>      moves = []
>      for m, l in actions.items():
>          l.sort()
> diff --git a/tests/test-backout.t b/tests/test-backout.t
> --- a/tests/test-backout.t
> +++ b/tests/test-backout.t
> @@ -686,6 +686,7 @@ Test usage of `hg resolve` in case of co
>    * version 2 records
>    local: b71750c4b0fdf719734971e3ef90dbeab5919a2d
>    other: a30dd8addae3ce71b8667868478542bc417439e6
> +  branch merge: True
>    file: foo (state "u", hash 0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33)
>      local path: foo (flags "")
>      ancestor path: foo (node f89532f44c247a0e993d63e3a734dd781ab04708)
> diff --git a/tests/test-merge-local.t b/tests/test-merge-local.t
> --- a/tests/test-merge-local.t
> +++ b/tests/test-merge-local.t
> @@ -63,6 +63,7 @@ Local merge with bad merge tool:
>    * version 2 records
>    local: c929647821fa73cdd20aa068da4153b2182c2731
>    other: 432e9bca7678bb78680640a7fd5143a8cb474b0f
> +  branch merge: False
>    file: zzz1_merge_ok (state "r", hash
> bad31184c814b16885cf966ecece6c1f2c255110)
>      local path: zzz1_merge_ok (flags "")
>      ancestor path: zzz1_merge_ok (node
> 5603a38019577b2389ad5a20dc73e0b40c25cab5)
> @@ -108,6 +109,7 @@ Local merge with conflicts:
>    * version 2 records
>    local: c929647821fa73cdd20aa068da4153b2182c2731
>    other: 432e9bca7678bb78680640a7fd5143a8cb474b0f
> +  branch merge: False
>    file: zzz1_merge_ok (state "r", hash
> bad31184c814b16885cf966ecece6c1f2c255110)
>      local path: zzz1_merge_ok (flags "")
>      ancestor path: zzz1_merge_ok (node
> 5603a38019577b2389ad5a20dc73e0b40c25cab5)
> @@ -154,6 +156,7 @@ Local merge without conflicts:
>    * version 2 records
>    local: c929647821fa73cdd20aa068da4153b2182c2731
>    other: 432e9bca7678bb78680640a7fd5143a8cb474b0f
> +  branch merge: False
>    file: zzz1_merge_ok (state "r", hash
> bad31184c814b16885cf966ecece6c1f2c255110)
>      local path: zzz1_merge_ok (flags "")
>      ancestor path: zzz1_merge_ok (node
> 5603a38019577b2389ad5a20dc73e0b40c25cab5)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
>
Siddharth Agarwal - Nov. 4, 2015, 1:28 a.m.
On 11/3/15 17:19, Martin von Zweigbergk wrote:
> I instead used a 'T' field to indicate the type of conflict, which 
> could be 'm', 'cd' or 'dc'. It's been too long for me to understand 
> what the pros and cons of that would be versus your approach of 
> storing the branch merge flag. What do you think?

My future patches will introduce something similar to that as well. The 
reason the branchmerge flag needs to be stored is related to dirstate 
modifications that need to be done based on whether this is a branch 
merge. For example, a local changed/remote deleted file needs to be 
marked added iff this isn't a branch merge.

>
> Here is my attempt, rebased part of the way to the current @: 
> http://42.netv6.net/martinvonz-wip/mercurial/log?rev=197a4ea5f82d%3A%3A89aa1f9405a5. 
> I don't even remember if there was some flaw in it or why I didn't 
> send it for review back then. It might be that the problems are now 
> fixed by the patches dealing with .hgsubstate that I saw a while back.
Martin von Zweigbergk - Nov. 4, 2015, 1:31 a.m.
On Tue, Nov 3, 2015 at 5:28 PM Siddharth Agarwal <sid@less-broken.com>
wrote:

> On 11/3/15 17:19, Martin von Zweigbergk wrote:
> > I instead used a 'T' field to indicate the type of conflict, which
> > could be 'm', 'cd' or 'dc'. It's been too long for me to understand
> > what the pros and cons of that would be versus your approach of
> > storing the branch merge flag. What do you think?
>
> My future patches will introduce something similar to that as well. The
> reason the branchmerge flag needs to be stored is related to dirstate
> modifications that need to be done based on whether this is a branch
> merge. For example, a local changed/remote deleted file needs to be
> marked added iff this isn't a branch merge.
>

Oh, I see. I see that I did "if repo.dirstate.p2() != nullid" in my patch.
Would that be better or worse?


>
> >
> > Here is my attempt, rebased part of the way to the current @:
> >
> http://42.netv6.net/martinvonz-wip/mercurial/log?rev=197a4ea5f82d%3A%3A89aa1f9405a5
> .
> > I don't even remember if there was some flaw in it or why I didn't
> > send it for review back then. It might be that the problems are now
> > fixed by the patches dealing with .hgsubstate that I saw a while back.
>
>
Siddharth Agarwal - Nov. 4, 2015, 1:35 a.m.
On 11/3/15 17:31, Martin von Zweigbergk wrote:
> Oh, I see. I see that I did "if repo.dirstate.p2() != nullid" in my 
> patch. Would that be better or worse?

Hmm, yeah, that'll probably work fine. Not sure how I missed that while 
digging into this stuff :)

All right, please drop these patches.

- Siddharth

Patch

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -60,6 +60,7 @@  class mergestate(object):
 
     L: the node of the "local" part of the merge (hexified version)
     O: the node of the "other" part of the merge (hexified version)
+    b: whether this is a branch merge
     F: a file to be merged entry
     D: a file that the external merge driver will merge internally
        (experimental)
@@ -80,7 +81,7 @@  class mergestate(object):
         self._dirty = False
         self._read()
 
-    def reset(self, node=None, other=None):
+    def reset(self, node=None, other=None, branchmerge=False):
         self._state = {}
         self._local = None
         self._other = None
@@ -89,6 +90,7 @@  class mergestate(object):
         if node:
             self._local = node
             self._other = other
+        self._branchmerge = branchmerge
         self._readmergedriver = None
         if self.mergedriver:
             self._mdstate = 's'
@@ -108,6 +110,7 @@  class mergestate(object):
         self._other = None
         if 'otherctx' in vars(self):
             del self.otherctx
+        self._branchmerge = False
         self._readmergedriver = None
         self._mdstate = 's'
         records = self._readrecords()
@@ -116,6 +119,8 @@  class mergestate(object):
                 self._local = bin(record)
             elif rtype == 'O':
                 self._other = bin(record)
+            elif rtype == 'b':
+                self._branchmerge = bool(int(record))
             elif rtype == 'm':
                 bits = record.split('\0', 1)
                 mdstate = bits[1]
@@ -269,6 +274,7 @@  class mergestate(object):
             records = []
             records.append(('L', hex(self._local)))
             records.append(('O', hex(self._other)))
+            records.append(('b', str(int(self._branchmerge))))
             if self.mergedriver:
                 records.append(('m', '\0'.join([
                     self.mergedriver, self._mdstate])))
@@ -837,7 +843,7 @@  def applyupdates(repo, actions, wctx, mc
 
     updated, merged, removed, unresolved = 0, 0, 0, 0
     ms = mergestate(repo)
-    ms.reset(wctx.p1().node(), mctx.node())
+    ms.reset(wctx.p1().node(), mctx.node(), branchmerge)
     moves = []
     for m, l in actions.items():
         l.sort()
diff --git a/tests/test-backout.t b/tests/test-backout.t
--- a/tests/test-backout.t
+++ b/tests/test-backout.t
@@ -686,6 +686,7 @@  Test usage of `hg resolve` in case of co
   * version 2 records
   local: b71750c4b0fdf719734971e3ef90dbeab5919a2d
   other: a30dd8addae3ce71b8667868478542bc417439e6
+  branch merge: True
   file: foo (state "u", hash 0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33)
     local path: foo (flags "")
     ancestor path: foo (node f89532f44c247a0e993d63e3a734dd781ab04708)
diff --git a/tests/test-merge-local.t b/tests/test-merge-local.t
--- a/tests/test-merge-local.t
+++ b/tests/test-merge-local.t
@@ -63,6 +63,7 @@  Local merge with bad merge tool:
   * version 2 records
   local: c929647821fa73cdd20aa068da4153b2182c2731
   other: 432e9bca7678bb78680640a7fd5143a8cb474b0f
+  branch merge: False
   file: zzz1_merge_ok (state "r", hash bad31184c814b16885cf966ecece6c1f2c255110)
     local path: zzz1_merge_ok (flags "")
     ancestor path: zzz1_merge_ok (node 5603a38019577b2389ad5a20dc73e0b40c25cab5)
@@ -108,6 +109,7 @@  Local merge with conflicts:
   * version 2 records
   local: c929647821fa73cdd20aa068da4153b2182c2731
   other: 432e9bca7678bb78680640a7fd5143a8cb474b0f
+  branch merge: False
   file: zzz1_merge_ok (state "r", hash bad31184c814b16885cf966ecece6c1f2c255110)
     local path: zzz1_merge_ok (flags "")
     ancestor path: zzz1_merge_ok (node 5603a38019577b2389ad5a20dc73e0b40c25cab5)
@@ -154,6 +156,7 @@  Local merge without conflicts:
   * version 2 records
   local: c929647821fa73cdd20aa068da4153b2182c2731
   other: 432e9bca7678bb78680640a7fd5143a8cb474b0f
+  branch merge: False
   file: zzz1_merge_ok (state "r", hash bad31184c814b16885cf966ecece6c1f2c255110)
     local path: zzz1_merge_ok (flags "")
     ancestor path: zzz1_merge_ok (node 5603a38019577b2389ad5a20dc73e0b40c25cab5)