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