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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 3725 bytes --]

Hi Jakub,

On Fri, 25 Nov 2016, Jakub Narębski wrote:

> W dniu 24.11.2016 o 21:55, Johannes Schindelin pisze:
> 
> > The current version of the builtin difftool does not, however, make
> > full use of the internals but instead chooses to spawn a couple of Git
> > processes, still, to make for an easier conversion. There remains a
> > lot of room for improvement, left for a later date.
> [...]
> 
> > Sadly, the speedup is more noticable on Linux than on Windows: a quick
> > test shows that t7800-difftool.sh runs in (2.183s/0.052s/0.108s)
> > (real/user/sys) in a Linux VM, down from  (6.529s/3.112s/0.644s),
> > while on Windows, it is (36.064s/2.730s/7.194s), down from
> > (47.637s/2.407s/6.863s). The culprit is most likely the overhead
> > incurred from *still* having to shell out to mergetool-lib.sh and
> > difftool--helper.sh.
> 
> Does this mean that our shell-based testsuite is not well suited to be
> benchmark suite for comparing performance on MS Windows?

It is quite likely the case that shell-based testing will always be
inappropriate for performance testing. Even on Linux.

> Or does it mean that "builtin-difftool" spawning Git processes is the
> problem?

At the moment I would have to guess, and I'd rather not.

> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  builtin/difftool.c | 670 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 669 insertions(+), 1 deletion(-)
> > 
> > diff --git a/builtin/difftool.c b/builtin/difftool.c
> > index 53870bb..3480920 100644
> > --- a/builtin/difftool.c
> > +++ b/builtin/difftool.c
> > @@ -11,9 +11,608 @@
> >   *
> >   * Copyright (C) 2016 Johannes Schindelin
> >   */
> > +#include "cache.h"
> >  #include "builtin.h"
> >  #include "run-command.h"
> >  #include "exec_cmd.h"
> > +#include "parse-options.h"
> > +#include "argv-array.h"
> > +#include "strbuf.h"
> > +#include "lockfile.h"
> > +#include "dir.h"
> > +
> > +static char *diff_gui_tool;
> > +static int trust_exit_code;
> > +
> > +static const char *const builtin_difftool_usage[] = {
> > +	N_("git difftool [<options>] [<commit> [<commit>]] [--] [<path>...]"),
> > +	NULL
> > +};
> > +
> > +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.

> > +static int print_tool_help(void)
> > +{
> > +	const char *argv[] = { "mergetool", "--tool-help=diff", NULL };
> > +	return run_command_v_opt(argv, RUN_GIT_CMD);
> 
> This looks a bit strange to me, but I guess this is to avoid recursively
> invoking ourself, and { "legacy-difftool", "--tool-help", NULL }; isn't
> that much better.

This is obviously a straight translation of the Perl script (see
https://github.com/git/git/blob/v2.10.2/git-difftool.perl#L40-L46):

	sub print_tool_help
	{
		# See the comment at the bottom of file_diff() for the reason
		# behind
		# using system() followed by exit() instead of exec().
		my $rc = system(qw(git mergetool --tool-help=diff));
		exit($rc | ($rc >> 8));
	}

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