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

From: Jakub Narębski <jnareb@gmail.com>
Date: Nov 27, 2016, 11:20 UTC
Message-ID: <CANQwDwcbsCTWsS8oFJaC8+dsLpqjivfxJt741G3QpCvir-_WAg@mail.gmail.com>
In-Reply-To: <alpine.DEB.2.20.1611261407520.117539@virtualbox>

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:

[...]

Show 10 quoted lines

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

[...]

Show 7 quoted lines

> 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