Patchwork mergetools: add nbdime merge tools to default rc

exporting patch:
# HG changeset patch
# User Vidar Tonaas Fauske
# Date 1510750467 -3600
#      Wed Nov 15 13:54:27 2017 +0100
# Node ID 7072463ab05996c7ae12ea4a6ccb9da305c54a53
# Parent  75013952d8d9608f73cd45f68405fbd6ec112bf2
mergetools: add nbdime merge tools to default rc

Adds default configuration for nbdime merge tools for the Jupyter notebook
format (a structured JSON document). If the user has nbdime installed, this
change will cause mercurial to use it by default (but with a low priority).

Why is it a good idea to use a different tool for Notebooks? Because the default
merge tool can produce invalid documents (non-valid JSON), meaning the user
has to edit the JSON by hand (which is not the intenion). The nbdime merge tool
ensures that the output is always a valid notebook, even when conflicted.

On Sat, 25 Nov 2017 13:34:58 +0100, Vidar Tonaas Fauske wrote:
> exporting patch:
> # HG changeset patch
> # User Vidar Tonaas Fauske
> # Date 1510750467 -3600
> #      Wed Nov 15 13:54:27 2017 +0100
> # Node ID 7072463ab05996c7ae12ea4a6ccb9da305c54a53
> # Parent  75013952d8d9608f73cd45f68405fbd6ec112bf2
> mergetools: add nbdime merge tools to default rc
> 
> Adds default configuration for nbdime merge tools for the Jupyter notebook
> format (a structured JSON document). If the user has nbdime installed, this
> change will cause mercurial to use it by default (but with a low priority).
> 
> Why is it a good idea to use a different tool for Notebooks? Because the default
> merge tool can produce invalid documents (non-valid JSON), meaning the user
> has to edit the JSON by hand (which is not the intenion). The nbdime merge tool
> ensures that the output is always a valid notebook, even when conflicted.
> 
> diff -r 75013952d8d9 -r 7072463ab059 mercurial/default.d/mergetools.rc
> --- a/mercurial/default.d/mergetools.rc Fri Nov 10 19:14:06 2017 +0800
> +++ b/mercurial/default.d/mergetools.rc Wed Nov 15 13:54:27 2017 +0100
> @@ -144,3 +144,24 @@
>  UltraCompare.binary = True
>  UltraCompare.check = conflicts,changed
>  UltraCompare.diffargs=$child $parent -title1 $clabel -title2 $plabel1
> +
> +# Nbdime for merging Jupyter notebooks:
> +#   merge driver:

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?

> +nbdime.priority = 2
> +nbdime.premerge = False
> +nbdime.executable = hg-nbmerge
> +nbdime.args = $base $local $other $output
> +#   merge tool:
> +nbdimeweb.priority = 1
> +nbdimeweb.premerge = False
> +nbdimeweb.executable = hg-nbmergeweb
> +nbdimeweb.args = --log-level ERROR $base $local $other $output
> +nbdimeweb.gui = True

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.

> +# 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?

On Sat, Nov 25, 2017 at 01:34:58PM +0100, Vidar Tonaas Fauske wrote:
> exporting patch:
> # HG changeset patch
> # User Vidar Tonaas Fauske
> # Date 1510750467 -3600
> #      Wed Nov 15 13:54:27 2017 +0100
> # Node ID 7072463ab05996c7ae12ea4a6ccb9da305c54a53
> # Parent  75013952d8d9608f73cd45f68405fbd6ec112bf2
> mergetools: add nbdime merge tools to default rc

seems like a reasonable default, queued, thanks

On Sun, Nov 26, 2017 at 08:33:58PM +0900, Yuya Nishihara wrote:
> On Sat, 25 Nov 2017 13:34:58 +0100, Vidar Tonaas Fauske wrote:
> > exporting patch:
> > # HG changeset patch
> > # User Vidar Tonaas Fauske
> > # Date 1510750467 -3600
> > #      Wed Nov 15 13:54:27 2017 +0100
> > # Node ID 7072463ab05996c7ae12ea4a6ccb9da305c54a53
> > # Parent  75013952d8d9608f73cd45f68405fbd6ec112bf2
> > mergetools: add nbdime merge tools to default rc
> >
> > Adds default configuration for nbdime merge tools for the Jupyter notebook
> > format (a structured JSON document). If the user has nbdime installed, this
> > change will cause mercurial to use it by default (but with a low priority).
> >
> > Why is it a good idea to use a different tool for Notebooks? Because the default
> > merge tool can produce invalid documents (non-valid JSON), meaning the user
> > has to edit the JSON by hand (which is not the intenion). The nbdime merge tool
> > ensures that the output is always a valid notebook, even when conflicted.
> >
> > diff -r 75013952d8d9 -r 7072463ab059 mercurial/default.d/mergetools.rc
> > --- a/mercurial/default.d/mergetools.rc Fri Nov 10 19:14:06 2017 +0800
> > +++ b/mercurial/default.d/mergetools.rc Wed Nov 15 13:54:27 2017 +0100
> > @@ -144,3 +144,24 @@
> >  UltraCompare.binary = True
> >  UltraCompare.check = conflicts,changed
> >  UltraCompare.diffargs=$child $parent -title1 $clabel -title2 $plabel1
> > +
> > +# Nbdime for merging Jupyter notebooks:
> > +#   merge driver:
>
> 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?
>
> > +nbdime.priority = 2
> > +nbdime.premerge = False
> > +nbdime.executable = hg-nbmerge
> > +nbdime.args = $base $local $other $output
> > +#   merge tool:
> > +nbdimeweb.priority = 1
> > +nbdimeweb.premerge = False
> > +nbdimeweb.executable = hg-nbmergeweb
> > +nbdimeweb.args = --log-level ERROR $base $local $other $output
> > +nbdimeweb.gui = True
>
> 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.
>
> > +# 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?

Ah, good point, I hadn't considered that. Will drop from -committed.
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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.

On Fri, 1 Dec 2017 11:25:01 +0100, Vidar Tonaas Fauske wrote:
> > 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.

Got it, thanks. Perhaps, "non-interactive/interactive merge" would be better
here.

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

Maybe -100 or -200? bdc8f048166e says "give worst-case 'merge' merge-tool
lowest priority", which is -100.

  $ grep priority mercurial/default.d/mergetools.rc | sort -t= -k2n
  merge.priority=-100
  vimdiff.priority=-10
  winmerge.priority=-10
  gvimdiff.priority=-9
  p4merge.priority=-8
  p4mergeosx.priority=-8
  tkdiff.priority=-8
  tortoisemerge.priority=-8
  xxdiff.priority=-8
  diffmerge.priority=-7
  diffuse.priority=-3
  UltraCompare.priority = -2
  araxis.priority=-2
  beyondcompare3.priority=-2
  bcompare.priority=-1
  bcomposx.priority=-1

> >> +# 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.

My understanding is that we ship the merge-tools settings by default since
defining them is cumbersome and they don't change the default behavior
(as long as the priority is low.) On the other hand, adding merge-patterns
does change the behavior. Also, it's as easy as setting ui.merge.