Patchwork [2,of,5] merge: define conflict marker labels in filemerge()

login
register
mail settings
Submitter Durham Goode
Date May 9, 2014, 12:33 a.m.
Message ID <7ff0390bd790e788b91b.1399595607@dev2000.prn2.facebook.com>
Download mbox | patch
Permalink /patch/4687/
State Accepted
Commit 20b8090d812531501c5205ff04d1f937f6ac912c
Headers show

Comments

Durham Goode - May 9, 2014, 12:33 a.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1399592253 25200
#      Thu May 08 16:37:33 2014 -0700
# Node ID 7ff0390bd790e788b91b85dee7fecd00d588ba4f
# Parent  86c73023576a450fd7c61a22cdb9fb82ad504e4b
merge: define conflict marker labels in filemerge()

Moves the conflict marker definition up to filemerge, so it gets applied to all
merge strategies, and so in a future patch we can manipulate the markers.
Mads Kiilerich - May 9, 2014, 10:36 a.m.
On 05/09/2014 02:33 AM, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1399592253 25200
> #      Thu May 08 16:37:33 2014 -0700
> # Node ID 7ff0390bd790e788b91b85dee7fecd00d588ba4f
> # Parent  86c73023576a450fd7c61a22cdb9fb82ad504e4b
> merge: define conflict marker labels in filemerge()
>
> Moves the conflict marker definition up to filemerge, so it gets applied to all
> merge strategies, and so in a future patch we can manipulate the markers.
>
> diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
> --- a/mercurial/filemerge.py
> +++ b/mercurial/filemerge.py
> @@ -169,7 +169,7 @@
>       used to resolve these conflicts."""
>       return 1
>   
> -def _premerge(repo, toolconf, files):
> +def _premerge(repo, toolconf, files, labels=None):

It seems like labels here (and perhaps even more in other places in this 
series) always is specified and more or less mandatory. Wouldn't it be 
better to be explicit and leave it without a default value?

/Mads
Durham Goode - May 9, 2014, 6:40 p.m.
On 5/9/14, 3:36 AM, "Mads Kiilerich" <mads@kiilerich.com> wrote:

>On 05/09/2014 02:33 AM, Durham Goode wrote:
>> # HG changeset patch
>> # User Durham Goode <durham@fb.com>
>> # Date 1399592253 25200
>> #      Thu May 08 16:37:33 2014 -0700
>> # Node ID 7ff0390bd790e788b91b85dee7fecd00d588ba4f
>> # Parent  86c73023576a450fd7c61a22cdb9fb82ad504e4b
>> merge: define conflict marker labels in filemerge()
>>
>> Moves the conflict marker definition up to filemerge, so it gets
>>applied to all
>> merge strategies, and so in a future patch we can manipulate the
>>markers.
>>
>> diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
>> --- a/mercurial/filemerge.py
>> +++ b/mercurial/filemerge.py
>> @@ -169,7 +169,7 @@
>>       used to resolve these conflicts."""
>>       return 1
>>   
>> -def _premerge(repo, toolconf, files):
>> +def _premerge(repo, toolconf, files, labels=None):
>
>It seems like labels here (and perhaps even more in other places in this
>series) always is specified and more or less mandatory. Wouldn't it be
>better to be explicit and leave it without a default value?
>
>/Mads
>

I can make it required for this patch (since these functions are generally
only called through filemerge).  For the other patch though (that touches
applyupdates), I wanted to minimize impact on extensions by not changing
the function signature too much.
Pierre-Yves David - May 9, 2014, 8:24 p.m.
On 05/09/2014 11:40 AM, Durham Goode wrote:
> On 5/9/14, 3:36 AM, "Mads Kiilerich" <mads@kiilerich.com> wrote:
>
>> On 05/09/2014 02:33 AM, Durham Goode wrote:
>>> # HG changeset patch
>>> # User Durham Goode <durham@fb.com>
>>> # Date 1399592253 25200
>>> #      Thu May 08 16:37:33 2014 -0700
>>> # Node ID 7ff0390bd790e788b91b85dee7fecd00d588ba4f
>>> # Parent  86c73023576a450fd7c61a22cdb9fb82ad504e4b
>>> merge: define conflict marker labels in filemerge()
>>>
>>> Moves the conflict marker definition up to filemerge, so it gets
>>> applied to all
>>> merge strategies, and so in a future patch we can manipulate the
>>> markers.
>>>
>>> diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
>>> --- a/mercurial/filemerge.py
>>> +++ b/mercurial/filemerge.py
>>> @@ -169,7 +169,7 @@
>>>        used to resolve these conflicts."""
>>>        return 1
>>>
>>> -def _premerge(repo, toolconf, files):
>>> +def _premerge(repo, toolconf, files, labels=None):
>>
>> It seems like labels here (and perhaps even more in other places in this
>> series) always is specified and more or less mandatory. Wouldn't it be
>> better to be explicit and leave it without a default value?
>>
>> /Mads
>>
>
> I can make it required for this patch (since these functions are generally
> only called through filemerge).  For the other patch though (that touches
> applyupdates), I wanted to minimize impact on extensions by not changing
> the function signature too much.

I think that having default value fallback is good to have.
Durham Goode - May 10, 2014, 1:50 a.m.
On 5/8/14, 5:33 PM, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1399592253 25200
> #      Thu May 08 16:37:33 2014 -0700
> # Node ID 7ff0390bd790e788b91b85dee7fecd00d588ba4f
> # Parent  86c73023576a450fd7c61a22cdb9fb82ad504e4b
> merge: define conflict marker labels in filemerge()
>
> Moves the conflict marker definition up to filemerge, so it gets applied to all
> merge strategies, and so in a future patch we can manipulate the markers.
>
Looks like these first two were queued by someone.

Patch

diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
--- a/mercurial/filemerge.py
+++ b/mercurial/filemerge.py
@@ -169,7 +169,7 @@ 
     used to resolve these conflicts."""
     return 1
 
-def _premerge(repo, toolconf, files):
+def _premerge(repo, toolconf, files, labels=None):
     tool, toolpath, binary, symlink = toolconf
     if symlink:
         return 1
@@ -190,7 +190,7 @@ 
                                     (tool, premerge, _valid))
 
     if premerge:
-        r = simplemerge.simplemerge(ui, a, b, c, quiet=True)
+        r = simplemerge.simplemerge(ui, a, b, c, quiet=True, label=labels)
         if not r:
             ui.debug(" premerge successful\n")
             return 0
@@ -201,7 +201,7 @@ 
 @internaltool('merge', True,
               _("merging %s incomplete! "
                 "(edit conflicts, then use 'hg resolve --mark')\n"))
-def _imerge(repo, mynode, orig, fcd, fco, fca, toolconf, files):
+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
@@ -211,19 +211,18 @@ 
         repo.ui.warn(_('warning: internal:merge cannot merge symlinks '
                        'for %s\n') % fcd.path())
         return False, 1
-
-    r = _premerge(repo, toolconf, files)
+    r = _premerge(repo, toolconf, files, labels=labels)
     if r:
         a, b, c, back = files
 
         ui = repo.ui
 
-        r = simplemerge.simplemerge(ui, a, b, c, label=['local', 'other'])
+        r = simplemerge.simplemerge(ui, a, b, c, label=labels)
         return True, r
     return False, 0
 
 @internaltool('dump', True)
-def _idump(repo, mynode, orig, fcd, fco, fca, toolconf, files):
+def _idump(repo, mynode, orig, fcd, fco, fca, toolconf, files, labels=None):
     """
     Creates three versions of the files to merge, containing the
     contents of local, other and base. These files can then be used to
@@ -231,7 +230,7 @@ 
     ``a.txt``, these files will accordingly be named ``a.txt.local``,
     ``a.txt.other`` and ``a.txt.base`` and they will be placed in the
     same directory as ``a.txt``."""
-    r = _premerge(repo, toolconf, files)
+    r = _premerge(repo, toolconf, files, labels=labels)
     if r:
         a, b, c, back = files
 
@@ -242,8 +241,8 @@ 
         repo.wwrite(fd + ".base", fca.data(), fca.flags())
     return False, r
 
-def _xmerge(repo, mynode, orig, fcd, fco, fca, toolconf, files):
-    r = _premerge(repo, toolconf, files)
+def _xmerge(repo, mynode, orig, fcd, fco, fca, toolconf, files, labels=None):
+    r = _premerge(repo, toolconf, files, labels=labels)
     if r:
         tool, toolpath, binary, symlink = toolconf
         a, b, c, back = files
@@ -327,8 +326,9 @@ 
 
     ui.debug("my %s other %s ancestor %s\n" % (fcd, fco, fca))
 
+    labels = ['local', 'other']
     needcheck, r = func(repo, mynode, orig, fcd, fco, fca, toolconf,
-                        (a, b, c, back))
+                        (a, b, c, back), labels=labels)
     if not needcheck:
         if r:
             if onfailure: