[PATCH 1 of 5 V4] merge: add conflict marker formatter (BC)

[Pierre-Yves David]

On 05/15/2014 01:52 PM, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham at fb.com>
> # Date 1399593022 25200
> #      Thu May 08 16:50:22 2014 -0700
> # Node ID 10d7bf67218dc9b26a838925410d646310fd6ed9
> # Parent  1ae3cd6f836c3c96ee3e9a872c8e966750910c2d
> merge: add conflict marker formatter (BC)
>
> Adds a conflict marker formatter that can produce custom conflict marker
> descriptions. It can be set via merge.conflictmarkertemplate. The old behavior
> can be used still by setting merge.mergemarkers=basic.

We have no `merge` section. We have a ui, merge-patterns and merge-tools 
section.

This should go in "ui" (as mergemarkers), or in "merge-tools" (as 
internal:merge.mergemarkers)

I feel like "ui" would be a better fit.


This smoothly bring use to the next topic: You did not document those 
option in the help (otherwise we would have spotted that the section 
they belong to did not existed)

It is important that you update the documentation about those two new 
options. Sorry for not spotting that sooner.

> The default format is similar to:
>
>    {node|short} {tag} {branch} {bookmarks} - {author}: "{desc|firstline}"
>
> And renders as:
>
>    contextblahblah
>    <<<<<<< local: c7fdd7ce4652 - durham: "Fix broken stuff in my feature branch"
>    line from my changes
>    =======
>    line from the other changes
>    >>>>>>> other: a3e55d7f4d38  master - sid0: "This is a commit to master th...
>    morecontextblahblah
>
> diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
> --- a/mercurial/filemerge.py
> +++ b/mercurial/filemerge.py
> @@ -7,7 +7,7 @@
>
>   from node import short
>   from i18n import _
> -import util, simplemerge, match, error
> +import util, simplemerge, match, error, templater, templatekw
>   import os, tempfile, re, filecmp
>
>   def _toolstr(ui, tool, part, default=""):
> @@ -269,6 +269,58 @@
>           return True, r
>       return False, 0
>
> +def _formatconflictmarker(repo, ctx, template, label, pad):
> +    """Applies the given template to the ctx, prefixed by the label.
> +
> +    Pad is the minimum width of the label prefix, so that multiple markers
> +    can have aligned templated parts.
> +    """
> +    if ctx.node() is None:
> +        ctx = ctx.p1()
> +
> +    props = templatekw.keywords.copy()
> +    props['templ'] = template
> +    props['ctx'] = ctx
> +    props['repo'] = repo
> +    templateresult = template('conflictmarker', **props)
> +
> +    label = ('%s:' % label).ljust(pad + 1)
> +    mark = '%s %s' % (label, templater.stringify(templateresult))
> +
> +    # The <<< marks add 8 to the length, and '...' adds three, so max
> +    # length of the actual marker is 69.
> +    maxlength = 80 - 8 - 3
> +    if len(mark) > maxlength:
> +        mark = mark[:maxlength] + '...'
> +    return mark
> +
> +_defaultconflictmarker = ('{node|short} ' +
> +    '{ifeq(tags, "tip", "", "{tags} ")}' +
> +    '{if(bookmarks, "{bookmarks} ")}' +
> +    '{ifeq(branch, "default", "", "{branch} ")}' +
> +    '- {author|user}: "{desc|firstline}"')
> +
> +def _formatlabels(repo, fcd, fco, labels):
> +    """Formats the given labels using the conflict marker template.
> +
> +    Returns a list of formatted labels.
> +    """
> +    cd = fcd.changectx()
> +    co = fco.changectx()
> +
> +    ui = repo.ui
> +    template = ui.config('merge', 'conflictmarkertemplate',
> +        _defaultconflictmarker)
> +    template = templater.parsestring(template, quoted=False)
> +    tmpl = templater.templater(None, cache={ 'conflictmarker' : template })
> +
> +    pad = max(len(labels[0]), len(labels[1]))
> +
> +    return [
> +        _formatconflictmarker(repo, cd, tmpl, labels[0], pad),
> +        _formatconflictmarker(repo, co, tmpl, labels[1], pad),
> +    ]
> +
>   def filemerge(repo, mynode, orig, fcd, fco, fca):
>       """perform a 3-way merge in the working directory
>
> @@ -326,9 +378,15 @@
>
>       ui.debug("my %s other %s ancestor %s\n" % (fcd, fco, fca))
>
> +    markerstyle = ui.configbool('merge', 'mergemarkers', 'detailed')

Ss you already had spotted configbool is not the function you are 
looking for. This highlight the lack of testing. But you have already 
seen all that. (pointing it to prevent another reviewer to see it)

>       labels = ['local', 'other']
> +    if markerstyle == 'basic':
> +        formattedlabels = labels
> +    else:

So if the value is unknown we used `detailed`. It make sense to have 
that as a default. Make sure the doc mention it.

> +        formattedlabels = _formatlabels(repo, fcd, fco, labels)
> +
>       needcheck, r = func(repo, mynode, orig, fcd, fco, fca, toolconf,
> -                        (a, b, c, back), labels=labels)
> +                        (a, b, c, back), labels=formattedlabels)
>       if not needcheck:
>           if r:
>               if onfailure:


Patches looks good to me otherwise

-- 
Pierre-Yves David