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