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

[David Aguilar <davvid@gmail.com>]

On Tue, Nov 22, 2016 at 06:01:23PM +0100, Johannes Schindelin wrote:
> This adds a builtin difftool that represents a conversion of the current
> Perl script version of the difftool.
> 
> The motivation is that Perl scripts are not at all native on Windows,
> and that `git difftool` therefore is pretty slow on that platform, when
> there is no good reason for it to be slow.
> 
> In addition, Perl does not really have access to Git's internals. That
> means that any script will always have to jump through unnecessary
> hoops.

Nice!

> 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.
> 
> Note: the original difftool is still called by `git difftool`. To get the
> new, experimental version, call `git builtin-difftool`. The reason: this
> new, experimental, builtin difftool will be shipped as part of Git for
> Windows v2.11.0, to allow for easier large-scale testing.

I like this plan.  I was going to ask for an environment
variable (to preset in git-cola) but since Git for Windows is
handling it then everyone benefits.

> diff --git a/builtin/builtin-difftool.c b/builtin/builtin-difftool.c
> new file mode 100644
> index 0000000..9feefcd
> --- /dev/null
> +++ b/builtin/builtin-difftool.c
> @@ -0,0 +1,680 @@
> +/*
> + * "git difftool" builtin command
> + *
> + * This is a wrapper around the GIT_EXTERNAL_DIFF-compatible
> + * git-difftool--helper script.
> + *
> + * This script exports GIT_EXTERNAL_DIFF and GIT_PAGER for use by git.
> + * The GIT_DIFF* variables are exported for use by git-difftool--helper.
> + *
> + * Any arguments that are unknown to this script are forwarded to 'git diff'.
> + *
> + * Copyright (C) 2016 Johannes Schindelin
> + */
> +#include "cache.h"
> +#include "builtin.h"
> +#include "parse-options.h"
> +#include "run-command.h"
> +#include "argv-array.h"
> +#include "strbuf.h"
> +#include "lockfile.h"
> +
> +static char *diff_gui_tool;
> +static int trust_exit_code;
> +
> +static const char * const builtin_difftool_usage[] = {
> +	N_("git add [<options>] [--] <pathspec>..."),
> +	NULL
> +};

The usage should probably say "difftool" (or "builtin-difftool").

> [...]
> +static void changed_files(struct hashmap *result, const char *index_path,
> +			  const char *workdir)
> +{
> +[...]
> +}
> +
> +#include "dir.h"

Can this mid-file #include go to the top of the file?

> +static int run_dir_diff(const char *extcmd, int symlinks,
> +			int argc, const char **argv)
> +{
> +	char tmpdir[PATH_MAX];
> +	struct strbuf info = STRBUF_INIT, lpath = STRBUF_INIT;
> +	struct strbuf rpath = STRBUF_INIT, buf = STRBUF_INIT;
> +	struct strbuf ldir = STRBUF_INIT, rdir = STRBUF_INIT;
> +	struct strbuf wtdir = STRBUF_INIT;
> +	size_t ldir_len, rdir_len, wtdir_len;
> +	struct cache_entry *ce = xcalloc(1, sizeof(ce) + PATH_MAX + 1);
> +	const char *workdir, *tmp;
> +	int ret = 0, i;
> +	FILE *fp;
> +	struct hashmap working_tree_dups, submodules, symlinks2;
> +	struct hashmap_iter iter;
> +	struct pair_entry *entry;
> +	enum object_type type;
> +	unsigned long size;
> +	struct index_state wtindex;
> +	struct checkout lstate, rstate;
> +	int rc, flags = RUN_GIT_CMD, err = 0;
> +	struct child_process child = CHILD_PROCESS_INIT;
> +	const char *helper_argv[] = { "difftool--helper", NULL, NULL, NULL };
> +	struct hashmap wt_modified, tmp_modified;
> +	int indices_loaded = 0;
> +
> +	setup_work_tree();
> +	workdir = get_git_work_tree();
> +
> +	/* Setup temp directories */
> +	tmp = getenv("TMPDIR");
> +	sprintf(tmpdir, "%s/git-difftool.XXXXXX", tmp ? tmp : "/tmp");

Maybe snprintf instead?

getenv() won't return anything longer than PATH_MAX for most
users, but users are weird.

> +	if (!mkdtemp(tmpdir))
> +		return error("could not create temporary directory");

Mention the tmpdir here?

> +	strbuf_addf(&ldir, "%s/left/", tmpdir);
> +	strbuf_addf(&rdir, "%s/right/", tmpdir);
> +	strbuf_addstr(&wtdir, workdir);
> +	if (!wtdir.len || !is_dir_sep(wtdir.buf[wtdir.len - 1]))
> +		strbuf_addch(&wtdir, '/');
> +	mkdir(ldir.buf, 0777);
> +	mkdir(rdir.buf, 0777);

Seeing the perl mkpath() default 0777 spelled out this way
makes me wonder whether 0700 would be safer.

The mkdtemp() above is already using 0700 so it's ok, but it
might be worth making it consistent (later, perhaps).

> [...]
> +int cmd_builtin_difftool(int argc, const char ** argv, const char * prefix)
> +{
> +	int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,
> +	    tool_help = 0;
> +	static char *difftool_cmd = NULL, *extcmd = NULL;
> +
> +	struct option builtin_difftool_options[] = {
> +		OPT_BOOL('g', "gui", &use_gui_tool,
> +			 N_("use `diff.guitool` instead of `diff.tool`")),
> +		OPT_BOOL('d', "dir-diff", &dir_diff,
> +			 N_("perform a full-directory diff")),
> +		{ OPTION_SET_INT, 'y', "no-prompt", &prompt, NULL,
> +			N_("do not prompt before launching a diff tool"),
> +			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 0},
> +		{ OPTION_SET_INT, 0, "prompt", &prompt, NULL, NULL,
> +			PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_HIDDEN,
> +			NULL, 1 },
> +		OPT_BOOL(0, "symlinks", &symlinks,
> +			 N_("use symlinks in dir-diff mode")),
> +		OPT_STRING('t', "tool", &difftool_cmd, N_("<tool>"),
> +			   N_("use the specified diff tool")),
> +		OPT_BOOL(0, "tool-help", &tool_help,
> +			 N_("print a list of diff tools that may be used with "
> +			    "`--tool`")),
> +		OPT_BOOL(0, "trust-exit-code", &trust_exit_code,
> +			 N_("make 'git-difftool' exit when an invoked diff "
> +			    "tool returns a non - zero exit code")),
> +		OPT_STRING('x', "extcmd", &extcmd, N_("<command>"),
> +			   N_("specify a custom command for viewing diffs")),
> +		OPT_END()
> +	};
> +
> +	symlinks = has_symlinks;
> +
> +	git_config(difftool_config, NULL);
> +
> +	argc = parse_options(argc, argv, prefix, builtin_difftool_options,
> +			     builtin_difftool_usage, PARSE_OPT_KEEP_UNKNOWN |
> +			     PARSE_OPT_KEEP_DASHDASH);
> +
> +	if (tool_help)
> +		return print_tool_help();
> +
> +	if (use_gui_tool && diff_gui_tool && *diff_gui_tool)
> +		setenv("GIT_DIFF_TOOL", diff_gui_tool, 1);
> +	else if (difftool_cmd) {
> +		if (*difftool_cmd)
> +			setenv("GIT_DIFF_TOOL", difftool_cmd, 1);
> +		else
> +			die(_("no <tool> given for --tool=<tool>"));
> +	}
> +
> +	if (extcmd) {
> +		if (*extcmd)
> +			setenv("GIT_DIFFTOOL_EXTCMD", extcmd, 1);
> +		else
> +			die(_("no <cmd> given for --extcmd=<cmd>"));
> +	}
> +
> +	setenv("GIT_DIFFTOOL_TRUST_EXIT_CODE",
> +	       trust_exit_code ? "true" : "false", 1);
> +
> +	/*
> +	 * In directory diff mode, 'git-difftool--helper' is called once
> +	 * to compare the a / b directories.In file diff mode, 'git diff'
> +	 * will invoke a separate instance of 'git-difftool--helper' for
> +	 * each file that changed.
> +	 */

Missing space after "." in the comment above.

> +	if (dir_diff)
> +		return run_dir_diff(extcmd, symlinks, argc, argv);
> +	return run_file_diff(prompt, argc, argv);
> +}
> diff --git a/git.c b/git.c
> index efa1059..eaa0f67 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 },
> +	{ "builtin-difftool", cmd_builtin_difftool, RUN_SETUP | NEED_WORK_TREE },
>  	{ "fast-export", cmd_fast_export, RUN_SETUP },
>  	{ "fetch", cmd_fetch, RUN_SETUP },
>  	{ "fetch-pack", cmd_fetch_pack, RUN_SETUP },

This isn't alphabetical anymore, but it actually is if you
consider that the final plan is to change "builtin-difftool" to
"difftool".

If we want to minimize that future diff we could name
cmd_builtin_difftool() as cmd_difftool() for consistency now so
that the future commit only needs to tweak the string here.
-- 
David