Re: [PATCH v2 1/1] difftool: add the builtin

[Junio C Hamano <gitster@pobox.com>]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> +	if (!strcmp(var, "core.usebuiltindifftool")) {
> +		use_builtin_difftool = git_config_bool(var, value);
> +		return 0;
> +	}

This no way belongs to the core set; difftool.usebuiltin would be
more appropriate.

> +	if (!use_builtin_difftool) {
> +		const char *path = mkpath("%s/git-legacy-difftool", git_exec_path());
> +
> +		if (sane_execvp(path, (char **)argv) < 0)
> +			die_errno("could not exec %s", path);
> +
> +		return 0;
> +	}
> + ...
> diff --git a/git-difftool.perl b/git-legacy-difftool.perl
> similarity index 100%
> rename from git-difftool.perl
> rename to git-legacy-difftool.perl
> diff --git a/git.c b/git.c
> index efa1059..0e6bbee 100644
> --- a/git.c
> +++ b/git.c
> @@ -424,6 +424,7 @@ static struct cmd_struct commands[] = {
>  	{ "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE },
>  	{ "diff-index", cmd_diff_index, RUN_SETUP },
>  	{ "diff-tree", cmd_diff_tree, RUN_SETUP },
> +	{ "difftool", cmd_difftool, RUN_SETUP | NEED_WORK_TREE },

Running set-up would mean that the spawning of legacy-difftool would
be done after you chdir(2) up to the root level of the working tree,
no?  I do not think you can safely add these two bits here until the
migration completes.

I doubt that setting core.usebuiltindifftool to false and running
the tool from a subdirectory and a pathspec work correctly with this
patch.  If running difftool from a subdirectory with a pathspec is
not tested in t7800, perhaps we should.

It is nice that we can now lose PERL prerequisite from t7800 ;-)

Thanks.