Hello Johannes,
On 27 November 2016 at 12:10, Johannes Schindelin
<Johannes.Schindelin@xxxxxx> 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