This is an archive of the discontinued Mercurial Phabricator instance.

merge-tools: when calling external merge tool, describe the resolve inputs
ClosedPublic

Authored by spectral on Oct 14 2018, 5:16 AM.

Details

Summary

It is a common complaint that a user will be running some operation (histedit,
rebase, evolve, etc.), get into a merge-conflict situation, and not understand
what they are seeing - it is possible that the merge tool is configured to
display the hash, but it's difficult for most merge tools to display a good
snippet of the description.

In the worst case, configuring this template will lead to output that is
immediately covered by a terminal application, maybe the user can hit ctrl-z to
see it. In the common case, the output will be in a terminal window and a GUI
program will start, and it should be possible to view both the terminal and the
GUI program at the same time.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

spectral created this revision.Oct 14 2018, 5:16 AM
spectral updated this revision to Diff 12106.Oct 14 2018, 5:25 AM
yuja added a subscriber: yuja.Oct 16 2018, 6:05 AM

+def _describemerge(ui, repo, env, toolpath, args):
+ template = ui.config('ui', 'pre-merge-tool-output-template')
+ if not template:
+ return
+
+ # Remove HG_ prefix from entries in env and lowercase them
+ def sanitizeenv(k):
+ if k.startswith('HG_'):
+ return k[3:].lower()
+ return k

Nit: HG_FILE should be called as path per
https://www.mercurial-scm.org/wiki/GenericTemplatingPlan#Dictionary

We might want to structure these variables as base.node|short instead of
base_node for example, but that would require more work.

+ data = {sanitizeenv(k): v for k, v in env.items()}
+ data['toolpath'] = toolpath
+ data['toolargs'] = args
+
+ # TODO: make all of this something that can be specified on a per-tool basis
+ template = templater.unquotestring(template)
+
+ fm = ui.formatter("extmerge", pycompat.byteskwargs({'template': template}))

^^^^^^^^^^^^^^^^^^^^

Unnecessary. It's bytes dict.

+ fm.data(repo=repo, **data)

^^^^^^

Here pycompat.strkwargs() would be needed.

And use fm.context(repo=repo) or fm.context(fctx=fcd) since repo is
unprintable.

+coreconfigitem('ui', 'pre-merge-tool-output-template',
+ default=None,
+)

Can you update the help/config.txt as well?

spectral planned changes to this revision.Oct 17 2018, 11:16 AM
In D5094#76457, @yuja wrote:

+def _describemerge(ui, repo, env, toolpath, args):
+ template = ui.config('ui', 'pre-merge-tool-output-template')
+ if not template:
+ return
+
+ # Remove HG_ prefix from entries in env and lowercase them
+ def sanitizeenv(k):
+ if k.startswith('HG_'):
+ return k[3:].lower()
+ return k

Nit: HG_FILE should be called as path per
https://www.mercurial-scm.org/wiki/GenericTemplatingPlan#Dictionary

Done.

We might want to structure these variables as base.node|short instead of
base_node for example, but that would require more work.

I tried to do this, producing this json output:

{
 "base": {"islink": false, "label": "base", "name": "base", "node": "TZ>fP͏~\u0019������%`������z3������)"},
 "local": {"islink": false, "label": "working copy", "name": "local", "node": "ot���G-������xq���\u0001������o1c������"},
 "other": {"islink": false, "label": "merge rev", "name": "other", "node": " ���`���\u0013s_\u0011������0������\u0002���������K������"},
 "path": "mercurial/dispatch.py",
 "toolargs": "/usr/local/google/home/spectral/src/hg/hg_work/mercurial/dispatch.py '/usr/local/google/tmp/tmp/dispatch~base.erx9yG.py' '/usr/local/google/tmp/tmp/dispatch~other.gbxRqg.py'",
 "toolpath": "/bin/false"
}

Unfortunately, this doesn't work super well, when using the following flags:

--config ui.pre-merge-tool-output-template='{myfunc(local)}' --config 'templatealias.myfunc(d)="{d.label}"'

I get this error:

hg: parse error: {'node': 'ot\xc4G-\xd0\xc3\x92\xdcxq\x97\x01\x90\xdbo1c\xb1\x95', 'name': 'local', 'islink': False, 'label': 'working copy'} is not a dictionary
(keyword 'local' does not support member operation)

I can fix this with a pretty simple patch to templateutil's makewrapped, making it return hybriddict or hybridlist if it receives a dict or a list. I couldn't find any other similar ways of causing this error, though, so it's possible I'm doing something wrong?

Things I tried:

≻ hg log -r . -T'{myfunc(dict(k1="hi", k2="bye"))}' --config 'templatealias.myfunc(d)="{d.k1}"'
hi

≻ hg log -r . -T'{myfunc(namespaces)}' --config 'templatealias.myfunc(d)="{d.tags}"'
tip

I looked through all functions that accept 'formatteropts' and couldn't find any hg command that produces a dict that's *not* in a list. I also couldn't identify any way of interacting with these dicts-inside-of-lists as a single unit (i.e. hg version -T'{extensions%"{extension|json}"}' doesn't work: 'extension' isn't a way of referring to the dictionary that's in the 'extensions' list. I have to do something like hg version -T'{extensions%dict(name, ver, bundled)|json)}' to re-create the dictionary.)

+ data = {sanitizeenv(k): v for k, v in env.items()}
+ data['toolpath'] = toolpath
+ data['toolargs'] = args
+
+ # TODO: make all of this something that can be specified on a per-tool basis
+ template = templater.unquotestring(template)
+
+ fm = ui.formatter("extmerge", pycompat.byteskwargs({'template': template}))

^^^^^^^^^^^^^^^^^^^^

Unnecessary. It's bytes dict.

Done.

+ fm.data(repo=repo, **data)

^^^^^^

Here pycompat.strkwargs() would be needed.

Done.

And use fm.context(repo=repo) or fm.context(fctx=fcd) since repo is
unprintable.

Done. Did both (fm.context(repo=repo, fctx=fcl))

+coreconfigitem('ui', 'pre-merge-tool-output-template',
+ default=None,
+)

Can you update the help/config.txt as well?

Will do before sending final version (marking as 'changes planned' for now - I'm going to be unavailable for several days).

yuja added a comment.Oct 19 2018, 8:31 AM
> We might want to structure these variables as `base.node|short` instead of
>  `base_node` for example, but that would require more work.

[...]

Unfortunately, this doesn't work super well, when using the following flags:

There isn't a building block for a mapping dict holding mapping dicts, and
the formatter API doesn't support such structure either.

Fortunately, we don't need a formatter here since we just want to apply
a template to a single item. So if we had a wrapper for a single mapping
dict, the templater can be rendered as follows:

props = {
    'base': templateutil.mappingdict({'ctx': fca.changectx(), 'fctx': fca,
                                      'label': baselabel})
    ...
}
cmdutil.rendertemplate(fcd.changectx(), tmpl, props)

And base.path, base.node, etc. should just work.

I'll post the dict wrapper if you like the idea.

class mappingdict(mappable, _mappingsequence):
    """Wrapper for a single template mapping

    This isn't a sequence in a way that the underlying dict won't be iterated
    as a dict, but shares most of the _mappingsequence functions.
    """

    def __init__(self, mapping, name=None, tmpl=None):
        super(mappingdict, self).__init__(name, tmpl)
        self._mapping = mapping

    def tomap(self, context):
        return self._mapping

    def tobool(self, context, mapping):
        # no idea when a template mapping should be considered an empty, but
        # a mapping dict should have at least one item in practice, so always
        # mark this as non-empty.
        return True

    def tovalue(self, context, mapping):
        return super(mappingdict, self).tovalue(context, mapping)[0]
spectral updated this revision to Diff 12369.Nov 1 2018, 7:01 PM
In D5094#77146, @yuja wrote:
> We might want to structure these variables as `base.node|short` instead of
>  `base_node` for example, but that would require more work.

[...]

Unfortunately, this doesn't work super well, when using the following flags:

There isn't a building block for a mapping dict holding mapping dicts, and
the formatter API doesn't support such structure either.
Fortunately, we don't need a formatter here since we just want to apply
a template to a single item. So if we had a wrapper for a single mapping
dict, the templater can be rendered as follows:

props = {
    'base': templateutil.mappingdict({'ctx': fca.changectx(), 'fctx': fca,
                                      'label': baselabel})
    ...
}
cmdutil.rendertemplate(fcd.changectx(), tmpl, props)

And base.path, base.node, etc. should just work.

Done.

I'll post the dict wrapper if you like the idea.

class mappingdict(mappable, _mappingsequence):
    """Wrapper for a single template mapping
    This isn't a sequence in a way that the underlying dict won't be iterated
    as a dict, but shares most of the _mappingsequence functions.
    """
    def __init__(self, mapping, name=None, tmpl=None):
        super(mappingdict, self).__init__(name, tmpl)
        self._mapping = mapping
    def tomap(self, context):
        return self._mapping
    def tobool(self, context, mapping):
        # no idea when a template mapping should be considered an empty, but
        # a mapping dict should have at least one item in practice, so always
        # mark this as non-empty.
        return True
    def tovalue(self, context, mapping):
        return super(mappingdict, self).tovalue(context, mapping)[0]

I copy/pasted this to D5211. :)

yuja added a comment.Nov 2 2018, 9:20 AM
>   class mappingdict(mappable, _mappingsequence):
>       """Wrapper for a single template mapping
>   
>       This isn't a sequence in a way that the underlying dict won't be iterated
>       as a dict, but shares most of the _mappingsequence functions.
>       """
>   
>       def __init__(self, mapping, name=None, tmpl=None):
>           super(mappingdict, self).__init__(name, tmpl)
>           self._mapping = mapping
>   
>       def tomap(self, context):
>           return self._mapping
>   
>       def tobool(self, context, mapping):
>           # no idea when a template mapping should be considered an empty, but
>           # a mapping dict should have at least one item in practice, so always
>           # mark this as non-empty.
>           return True
>   
>       def tovalue(self, context, mapping):
>           return super(mappingdict, self).tovalue(context, mapping)[0]
I copy/pasted this to https://phab.mercurial-scm.org/D5211. :)

I sent my version with {p1}/{p2} example.

You don't need to update this series. I'll queue it once my patches are
accepted.

This revision was automatically updated to reflect the committed changes.