[PATCH] mergetools: add nbdime merge tools to default rc

Vidar Tonaas Fauske vidartf at gmail.com
Fri Dec 1 10:25:01 UTC 2017


Thanks for your reply! I've tried to address your comments point by point below:


> Nit: I'm not sure what this "merge driver" and "merge tool" mean, and the
> term "merge driver" seems to have a different meaning in Mercurial. Can you
> elaborate?


I wasn't certain of the mercurial terminology, so I just kept what I
had. Here "driver" is intended as something that runs unsupervised by
default for merges, inserting markers for conflicts (i.e. the `merge`
command). "Tool" is here meant as an application for resolving such
conflicts (i.e. called by the `resolve` command), in this case a web
GUI based one. If you can indicate what the terminology for mercurial
is, I'll make the appropriate changes.

The "driver" is here quite important for notebooks given that they are
structured documents. A line-based text merge is likely to result in
an invalid document, which is unfortunate since the internal JSON is
not generally meant for human consumption.


> These priorities seem too high compared to the other tools. I think these
> tools should have quite low priority so they won't be selected as a general-
> purpose merge tool.


Most of the current tools registered have a priority in the range -10
to -1, but these are general purpose tools, not a file-format specific
ones (as far as I can tell). What would be a reasonable priority for a
file-format specific merge tool? I was not able to find any prior
examples on this.


>
>> +# Default association of file types with merge tools
>> +
>> +[merge-patterns]
>> +
>> +**.ipynb = nbdime
>
> Registering merge-tools by default is good, but I don't think we should force
> users to select them by default. Can you send new patch that only registers
> merge-tools?


Again, there seems to be no prior pattern to follow for file-format
specific tools. Given that this is a file-format specific "driver", it
should only be called for the given file extension. As such the
`merge-patterns` entry seems mandatory for it to work as intended
(calling `hg merge --tool=nbdime` will cause it to receive any
file-types involved in the merge, not just notebooks, AFAICT). If the
user would have to edit the configuration to enable the tool (i.e. to
add the merge-patterns entry), there seems to be little meaning to
adding the "driver" without this line (this defaults exist so that
they can be used without editing the configuration). While I could
send a new patch without the driver, I would say that having the
"resolver" configured without the "driver" is an unlikely wanted setup
for end users.


More information about the Mercurial-devel mailing list