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

# 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.

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
>

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.

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.
>
>

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