Re: [PATCH v3 2/2] difftool: implement the functionality in the builtin

["Jakub Narębski" <jnareb@gmail.com>]

Hello Johannes,

On 27 November 2016 at 12:10, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Fri, 25 Nov 2016, Jakub Narębski wrote:
> > W dniu 24.11.2016 o 21:55, Johannes Schindelin pisze:

[...]
> > > +static int difftool_config(const char *var, const char *value, void *cb)
> > > +{
> > > +   if (!strcmp(var, "diff.guitool")) {
> >
> > Shouldn't you also read other configuration variables, like "diff.tool",
> > and it's mergetool fallbacks ("merge.guitool", "merge.tool")?
>
> No, as those configuration variables are not used by the builtin difftool
> directly but read by subsequently spawned commands separately. There would
> be no use reading them here, for now.

Ah, all right then.

Though NEEDSWORK comment would be nice to have here (for when
we don't spawn commands).

[...]
> I read the rest of your review, but it appears that it is more about
> style than about substance, while I am only willing to address the latter
> issues at the moment. You see, I want to focus on getting difftool correct
> first before attempting to make it pretty.
>
> Ciao,
> Dscho

Well, excet for the submodule-relates stuff, which I have skipped,
it looks good to me.
-- 
Jakub Narębski