Patchwork [V2] merge: Add support for 'union' merge strategy

login
register
mail settings
Submitter Erik Huelsmann
Date July 10, 2015, 6:10 p.m.
Message ID <CACOoB6gEWJZen9144COJW+6g1CUcSYZsu8iUQ8uTkzaw97jAeQ@mail.gmail.com>
Download mbox | patch
Permalink /patch/9950/
State Changes Requested
Headers show

Comments

Erik Huelsmann - July 10, 2015, 6:10 p.m.
# HG changeset patch
# User Erik Huelsmann <ehuels@gmail.com>
# Date 1435346089 -7200
#      Fri Jun 26 21:14:49 2015 +0200
# Node ID c8b0c9fb18ec813244b100e7ecf9ee5eb7b92f88
# Parent  ff5172c830022b64cc5bd1bae36b2276e9dc6e5d
merge: add support for 'union' merge strategy to internal merge tool.

'union' merge is a merge strategy where the left and right side of a
conflicting merge are merged into the target without generating a conflict.

One use-case for this merge strategy is the Changelog file being changed
on multiple branches and conflicting when being merged back to the main
branch.

The idea for this merge strategy has been taken from Git.

       default not handle symlinks or binary files.

diff -r ff5172c83002 -r c8b0c9fb18ec
tests/test-merge-internal-tools-pattern.t
--- a/tests/test-merge-internal-tools-pattern.t Wed Jun 24 13:41:27 2015
-0500
+++ b/tests/test-merge-internal-tools-pattern.t Fri Jun 26 21:14:49 2015
+0200
@@ -1,5 +1,6 @@
-Make sure that the internal merge tools (internal:fail, internal:local, and
-internal:other) are used when matched by a merge-pattern in hgrc
+Make sure that the internal merge tools (internal:fail, internal:local,
+internal:union and internal:other) are used when matched by a
+merge-pattern in hgrc

 Make sure HGMERGE doesn't interfere with the test:

@@ -110,3 +111,31 @@
   $ hg stat
   M f

+Merge using internal:union tool:
+
+  $ hg update -C 2
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+
+  $ echo "line 4a" >>f
+  $ hg ci -Am "Adding fourth line (commit 4)"
+  $ hg update 2
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+
+  $ echo "line 4b" >>f
+  $ hg ci -Am "Adding fourth line v2 (commit 5)"
+  created new head
+
+  $ echo "[merge-patterns]" > .hg/hgrc
+  $ echo "* = internal:union" >> .hg/hgrc
+
+  $ hg merge 3
+  merging f
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  (branch merge, don't forget to commit)
+
+  $ cat f
+  line 1
+  line 2
+  third line
+  line 4b
+  line 4a
Matt Mackall - July 10, 2015, 11:25 p.m.
On Fri, 2015-07-10 at 20:10 +0200, Erik Huelsmann wrote:
> # HG changeset patch
> # User Erik Huelsmann <ehuels@gmail.com>
> # Date 1435346089 -7200
> #      Fri Jun 26 21:14:49 2015 +0200
> # Node ID c8b0c9fb18ec813244b100e7ecf9ee5eb7b92f88
> # Parent  ff5172c830022b64cc5bd1bae36b2276e9dc6e5d
> merge: add support for 'union' merge strategy to internal merge tool.
> 
> 'union' merge is a merge strategy where the left and right side of a
> conflicting merge are merged into the target without generating a conflict.
> 
> One use-case for this merge strategy is the Changelog file being changed
> on multiple branches and conflicting when being merged back to the main
> branch.
> 
> The idea for this merge strategy has been taken from Git.

This looks ok, but it's a bit much going on in one patch by our
standards. Could I get you to split it up into the following pieces:

- bits that add start/end_marker (but please, no _ in new names)
- bits that union option to simplemerge
- bits that split out filemerge helper function
- bits that add union merge + test of same

Also, we may want the start/end marker business to be fully internal to
simplemerge rather than passing in more args.
Matt Mackall - July 11, 2015, midnight
On Fri, 2015-07-10 at 20:10 +0200, Erik Huelsmann wrote:
> # HG changeset patch
> # User Erik Huelsmann <ehuels@gmail.com>
> # Date 1435346089 -7200
> #      Fri Jun 26 21:14:49 2015 +0200
> # Node ID c8b0c9fb18ec813244b100e7ecf9ee5eb7b92f88
> # Parent  ff5172c830022b64cc5bd1bae36b2276e9dc6e5d
> merge: add support for 'union' merge strategy to internal merge tool.
> 
> 'union' merge is a merge strategy where the left and right side of a
> conflicting merge are merged into the target without generating a conflict.
> 
> One use-case for this merge strategy is the Changelog file being changed
> on multiple branches and conflicting when being merged back to the main
> branch.
> 
> The idea for this merge strategy has been taken from Git.

This looks ok, but it's a bit much going on in one patch by our
standards. Could I get you to split it up into the following pieces:

- bits that add start/end_marker (but please, no _ in new names)
- bits that union option to simplemerge
- bits that split out filemerge helper function
- bits that add union merge + test of same

Also, we may want the start/end marker business to be fully internal to
simplemerge rather than passing extra arguments.
Erik Huelsmann - July 11, 2015, 9:53 a.m.
Hi Matt,

On Sat, Jul 11, 2015 at 1:25 AM, Matt Mackall <mpm@selenic.com> wrote:

> On Fri, 2015-07-10 at 20:10 +0200, Erik Huelsmann wrote:
> > # HG changeset patch
> > # User Erik Huelsmann <ehuels@gmail.com>
> > # Date 1435346089 -7200
> > #      Fri Jun 26 21:14:49 2015 +0200
> > # Node ID c8b0c9fb18ec813244b100e7ecf9ee5eb7b92f88
> > # Parent  ff5172c830022b64cc5bd1bae36b2276e9dc6e5d
> > merge: add support for 'union' merge strategy to internal merge tool.
> >
> > 'union' merge is a merge strategy where the left and right side of a
> > conflicting merge are merged into the target without generating a
> conflict.
> >
> > One use-case for this merge strategy is the Changelog file being changed
> > on multiple branches and conflicting when being merged back to the main
> > branch.
> >
> > The idea for this merge strategy has been taken from Git.
>
> This looks ok, but it's a bit much going on in one patch by our
> standards. Could I get you to split it up into the following pieces:
>
> - bits that add start/end_marker (but please, no _ in new names)
> - bits that union option to simplemerge
> - bits that split out filemerge helper function
> - bits that add union merge + test of same
>

Ok. I can split it up that way, no problem. Just verifying before diving
in: are you expecting it as a set of patches ('1 of 4'...)? Each with the
same commit text as I provided above? Or are you expecting the last patch
with the commit text as above, and the others with a shorter description
like "In preparation of implementing union merge, rearrange ...."?


Also, we may want the start/end marker business to be fully internal to
> simplemerge rather than passing in more args.
>

Consider it done! :-)

I'm a bit spotty on time availability, but I'll get to resubmitting the
patches; hopefully without too much delay.

Thanks for the prompt response!
Augie Fackler - July 11, 2015, 6:17 p.m.
> On Jul 11, 2015, at 5:53 AM, Erik Huelsmann <ehuels@gmail.com> wrote:
> 
>> The idea for this merge strategy has been taken from Git.
> 
> This looks ok, but it's a bit much going on in one patch by our
> standards. Could I get you to split it up into the following pieces:
> 
> - bits that add start/end_marker (but please, no _ in new names)
> - bits that union option to simplemerge
> - bits that split out filemerge helper function
> - bits that add union merge + test of same
> 
> Ok. I can split it up that way, no problem. Just verifying before diving in: are you expecting it as a set of patches ('1 of 4'...)? Each with the same commit text as I provided above? Or are you expecting the last patch with the commit text as above, and the others with a shorter description like "In preparation of implementing union merge, rearrange ....”?

The latter - each commit message should describe what that commit is doing.

> 
>> 
>> Also, we may want the start/end marker business to be fully internal to
>> simplemerge rather than passing in more args.
> 
> Consider it done! :-)
> 
> I'm a bit spotty on time availability, but I'll get to resubmitting the patches; hopefully without too much delay.
> 
> Thanks for the prompt response!
>
Matt Harbison - July 11, 2015, 6:18 p.m.
On Sat, 11 Jul 2015 05:53:19 -0400, Erik Huelsmann <ehuels@gmail.com>  
wrote:

> Hi Matt,
>
> On Sat, Jul 11, 2015 at 1:25 AM, Matt Mackall <mpm@selenic.com> wrote:
>
>> On Fri, 2015-07-10 at 20:10 +0200, Erik Huelsmann wrote:
>> > # HG changeset patch
>> > # User Erik Huelsmann <ehuels@gmail.com>
>> > # Date 1435346089 -7200
>> > #      Fri Jun 26 21:14:49 2015 +0200
>> > # Node ID c8b0c9fb18ec813244b100e7ecf9ee5eb7b92f88
>> > # Parent  ff5172c830022b64cc5bd1bae36b2276e9dc6e5d
>> > merge: add support for 'union' merge strategy to internal merge tool.
>> >
>> > 'union' merge is a merge strategy where the left and right side of a
>> > conflicting merge are merged into the target without generating a
>> conflict.
>> >
>> > One use-case for this merge strategy is the Changelog file being  
>> changed
>> > on multiple branches and conflicting when being merged back to the  
>> main
>> > branch.
>> >
>> > The idea for this merge strategy has been taken from Git.
>>
>> This looks ok, but it's a bit much going on in one patch by our
>> standards. Could I get you to split it up into the following pieces:
>>
>> - bits that add start/end_marker (but please, no _ in new names)
>> - bits that union option to simplemerge
>> - bits that split out filemerge helper function
>> - bits that add union merge + test of same
>>
>
> Ok. I can split it up that way, no problem. Just verifying before diving
> in: are you expecting it as a set of patches ('1 of 4'...)?

Yes.  Get everything fixed up and tested, and patchbomb will handle the  
details if you give it the proper revisions.

> Each with the
> same commit text as I provided above? Or are you expecting the last patch
> with the commit text as above, and the others with a shorter description
> like "In preparation of implementing union merge, rearrange ...."?

Generally, each individual patch should summarize what _it_ does.  You can  
mention that it is a step towards X in the body if that isn't clear.  Scan  
back even 50 or so commits in the hg repo, and you will see several series  
by the same author to get the idea.

>
> Also, we may want the start/end marker business to be fully internal to
>> simplemerge rather than passing in more args.
>>
>
> Consider it done! :-)
>
> I'm a bit spotty on time availability, but I'll get to resubmitting the
> patches; hopefully without too much delay.

Just an FYI, the code freeze is in a week, after which it will have to  
wait until Aug 1.

> Thanks for the prompt response!
Erik Huelsmann - July 17, 2015, 12:38 p.m.
>
>> I'm a bit spotty on time availability, but I'll get to resubmitting the
>> patches; hopefully without too much delay.
>>
>
> Just an FYI, the code freeze is in a week, after which it will have to
> wait until Aug 1.
>

Since it's my first submission, I'm going to take my time and resubmit
after Aug 1st. Thanks for the warning!
Erik Huelsmann - Aug. 15, 2015, 9:38 p.m.
On Sat, Jul 11, 2015 at 1:25 AM, Matt Mackall <mpm@selenic.com> wrote:

> On Fri, 2015-07-10 at 20:10 +0200, Erik Huelsmann wrote:
> > # HG changeset patch
> > # User Erik Huelsmann <ehuels@gmail.com>
> > # Date 1435346089 -7200
> > #      Fri Jun 26 21:14:49 2015 +0200
> > # Node ID c8b0c9fb18ec813244b100e7ecf9ee5eb7b92f88
> > # Parent  ff5172c830022b64cc5bd1bae36b2276e9dc6e5d
> > merge: add support for 'union' merge strategy to internal merge tool.
> >
> > 'union' merge is a merge strategy where the left and right side of a
> > conflicting merge are merged into the target without generating a
> conflict.
> >
> > One use-case for this merge strategy is the Changelog file being changed
> > on multiple branches and conflicting when being merged back to the main
> > branch.
> >
> > The idea for this merge strategy has been taken from Git.
>
> This looks ok, but it's a bit much going on in one patch by our
> standards. Could I get you to split it up into the following pieces:
>
> - bits that add start/end_marker (but please, no _ in new names)
> - bits that union option to simplemerge
> - bits that split out filemerge helper function
> - bits that add union merge + test of same
>
> Also, we may want the start/end marker business to be fully internal to
> simplemerge rather than passing in more args.
>

Ok. But the labels were always external from simple merge already. Even
worse(?): filemerge offers the lables to its callers to set. Do you want me
to look at options to make the labels internal to simple merge?

Note that the labels are *not* the usual ">>>>>", "=======" and "<<<<<<"
labels, but they are text to be added *after* the labels. E.g. "local",
"other", etc. I wouldn't directly know how to keep that local, so I'll have
to look at it. There's a new 'mode' keyword arg which, if present with a
value of 'union', suppresses the markers. The suppression happens inside of
simplemerge. Would that satisfy your "start/end marker business fully
internal to simplemerge"?

With respect to the remark of the underscores: I wasn't creating new ones
(I think), but simply re-using variable names that were already used in the
file. I assumed I could use existing variables with underscores.


Thanks for your further guidance!

Patch

diff -r ff5172c83002 -r c8b0c9fb18ec mercurial/filemerge.py
--- a/mercurial/filemerge.py Wed Jun 24 13:41:27 2015 -0500
+++ b/mercurial/filemerge.py Fri Jun 26 21:14:49 2015 +0200
@@ -212,10 +212,7 @@ 
             util.copyfile(back, a) # restore from backup and try again
     return 1 # continue merging

-@internaltool('merge', True,
-              _("merging %s incomplete! "
-                "(edit conflicts, then use 'hg resolve --mark')\n"))
-def _imerge(repo, mynode, orig, fcd, fco, fca, toolconf, files,
labels=None):
+def __imerge(repo, mynode, orig, fcd, fco, fca, toolconf, files, labels,
mode):
     """
     Uses the internal non-interactive simple merge algorithm for merging
     files. It will fail if there are any conflicts and leave markers in
@@ -232,10 +229,38 @@ 

         ui = repo.ui

-        r = simplemerge.simplemerge(ui, a, b, c, label=labels)
+        r = simplemerge.simplemerge(ui, a, b, c, label=labels, mode=mode)
         return True, r
     return False, 0

+
+@internaltool('merge', True,
+              _("merging %s incomplete! "
+                "(edit conflicts, then use 'hg resolve --mark')\n"))
+def _imerge(repo, mynode, orig, fcd, fco, fca, toolconf, files,
labels=None):
+    """
+    Uses the internal non-interactive simple merge algorithm for merging
+    files. It will fail if there are any conflicts and leave markers in
+    the partially merged file. Markers will have two sections, one for
each side
+    of merge."""
+
+    return __imerge(repo, mynode, orig, fcd, fco, fca, toolconf,
+                    files, labels, 'merge')
+
+
+@internaltool('union', True,
+              _("merging %s incomplete! "
+                "(edit conflicts, then use 'hg resolve --mark')\n"))
+def _imerge(repo, mynode, orig, fcd, fco, fca, toolconf, files,
labels=None):
+    """
+    Uses the internal non-interactive union merge algorithm for merging
+    files. It will use both sides if there are any conflicts."""
+
+    return __imerge(repo, mynode, orig, fcd, fco, fca, toolconf,
+                    files, labels, 'union')
+
+
+
 @internaltool('merge3', True,
               _("merging %s incomplete! "
                 "(edit conflicts, then use 'hg resolve --mark')\n"))
diff -r ff5172c83002 -r c8b0c9fb18ec mercurial/simplemerge.py
--- a/mercurial/simplemerge.py Wed Jun 24 13:41:27 2015 -0500
+++ b/mercurial/simplemerge.py Fri Jun 26 21:14:49 2015 +0200
@@ -92,9 +92,9 @@ 
                 newline = '\r\n'
             elif self.a[0].endswith('\r'):
                 newline = '\r'
-        if name_a:
+        if name_a and start_marker:
             start_marker = start_marker + ' ' + name_a
-        if name_b:
+        if name_b and end_marker:
             end_marker = end_marker + ' ' + name_b
         if name_base and base_marker:
             base_marker = base_marker + ' ' + name_base
@@ -112,17 +112,20 @@ 
                     yield self.b[i]
             elif what == 'conflict':
                 self.conflicts = True
-                yield start_marker + newline
+                if start_marker is not None:
+                    yield start_marker + newline
                 for i in range(t[3], t[4]):
                     yield self.a[i]
                 if base_marker is not None:
                     yield base_marker + newline
                     for i in range(t[1], t[2]):
                         yield self.base[i]
-                yield mid_marker + newline
+                if mid_marker is not None:
+                    yield mid_marker + newline
                 for i in range(t[5], t[6]):
                     yield self.b[i]
-                yield end_marker + newline
+                if end_marker is not None:
+                    yield end_marker + newline
             else:
                 raise ValueError(what)

@@ -345,18 +348,24 @@ 
                 raise util.Abort(msg)
         return text

-    name_a = local
-    name_b = other
-    name_base = None
-    labels = opts.get('label', [])
-    if len(labels) > 0:
-        name_a = labels[0]
-    if len(labels) > 1:
-        name_b = labels[1]
-    if len(labels) > 2:
-        name_base = labels[2]
-    if len(labels) > 3:
-        raise util.Abort(_("can only specify three labels."))
+    mode = opts.get('mode', '')
+    if mode == 'union':
+        name_a = None
+        name_b = None
+        name_base = None
+    else:
+        name_a = local
+        name_b = other
+        name_base = None
+        labels = opts.get('label', [])
+        if len(labels) > 0:
+            name_a = labels[0]
+        if len(labels) > 1:
+            name_b = labels[1]
+        if len(labels) > 2:
+            name_base = labels[2]
+        if len(labels) > 3:
+            raise util.Abort(_("can only specify three labels."))

     try:
         localtext = readfile(local)
@@ -374,7 +383,11 @@ 

     m3 = Merge3Text(basetext, localtext, othertext)
     extrakwargs = {}
-    if name_base is not None:
+    if mode == 'union':
+        extrakwargs['start_marker'] = None
+        extrakwargs['end_marker'] = None
+        extrakwargs['mid_marker'] = None
+    elif name_base is not None:
         extrakwargs['base_marker'] = '|||||||'
         extrakwargs['name_base'] = name_base
     for line in m3.merge_lines(name_a=name_a, name_b=name_b,
**extrakwargs):
@@ -383,7 +396,7 @@ 
     if not opts.get('print'):
         out.close()

-    if m3.conflicts:
+    if m3.conflicts and not mode == 'union':
         if not opts.get('quiet'):
             ui.warn(_("warning: conflicts during merge.\n"))
         return 1
diff -r ff5172c83002 -r c8b0c9fb18ec tests/test-help.t
--- a/tests/test-help.t Wed Jun 24 13:41:27 2015 -0500
+++ b/tests/test-help.t Fri Jun 26 21:14:49 2015 +0200
@@ -1195,6 +1195,10 @@ 
       ":tagmerge"
         Uses the internal tag merge algorithm (experimental).

+       ":union"
+         Uses the internal non-interactive union merge algorithm for
merging
+         files. It will use both sides if there are any conflicts.
+
       Internal tools are always available and do not require a GUI but
will by