[PATCH v3 0/2] Show Git Mailing List: a builtin difftool

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

I have been working on the builtin difftool for almost two weeks,
for two reasons:

1. Perl is really not native on Windows. Not only is there a performance
   penalty to be paid just for running Perl scripts, we also have to deal
   with the fact that users may have different Perl installations, with
   different options, and some other Perl installation may decide to set
   PERL5LIB globally, wreaking havoc with Git for Windows' Perl (which we
   have to use because almost all other Perl distributions lack the
   Subversion bindings we need for `git svn`).

2. Perl makes for a rather large reason that Git for Windows' installer
   weighs in with >30MB. While one Perl script less does not relieve us
   of that burden, it is one step in the right direction.

This patch serves two purposes: to ask for reviews, and to show what I
plan to release as part of Git for Windows v2.11.0 (which is due this
coming Wednesday, if Git v2.11.0 is released on Tuesday, as planned).

Changes since v2:

- adjusted the config setting's name according to Junio's concerns

- fixed launching difftool in a subdirectory

- fixed dir-diff mode when there are no changes (it did not exit early
  but tried to diff two empty directories)


Johannes Schindelin (2):
  difftool: add a skeleton for the upcoming builtin
  difftool: implement the functionality in the builtin

 .gitignore                                    |   1 +
 Makefile                                      |   3 +-
 builtin.h                                     |   1 +
 builtin/difftool.c                            | 731 ++++++++++++++++++++++++++
 git-difftool.perl => git-legacy-difftool.perl |   0
 git.c                                         |   6 +
 t/t7800-difftool.sh                           |   2 +
 7 files changed, 743 insertions(+), 1 deletion(-)
 create mode 100644 builtin/difftool.c
 rename git-difftool.perl => git-legacy-difftool.perl (100%)


base-commit: e2b2d6a172b76d44cb7b1ddb12ea5bfac9613a44
Published-As: https://github.com/dscho/git/releases/tag/builtin-difftool-v3
Fetch-It-Via: git fetch https://github.com/dscho/git builtin-difftool-v3

Interdiff vs v2:

 diff --git a/builtin/difftool.c b/builtin/difftool.c
 index f845879..3480920 100644
 --- a/builtin/difftool.c
 +++ b/builtin/difftool.c
 @@ -13,17 +13,16 @@
   */
  #include "cache.h"
  #include "builtin.h"
 -#include "parse-options.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"
 -#include "exec_cmd.h"
  
  static char *diff_gui_tool;
  static int trust_exit_code;
 -static int use_builtin_difftool;
  
  static const char *const builtin_difftool_usage[] = {
  	N_("git difftool [<options>] [<commit> [<commit>]] [--] [<path>...]"),
 @@ -42,11 +41,6 @@ static int difftool_config(const char *var, const char *value, void *cb)
  		return 0;
  	}
  
 -	if (!strcmp(var, "core.usebuiltindifftool")) {
 -		use_builtin_difftool = git_config_bool(var, value);
 -		return 0;
 -	}
 -
  	return git_default_config(var, value, cb);
  }
  
 @@ -257,7 +251,7 @@ static int ensure_leading_directories(char *path)
  	}
  }
  
 -static int run_dir_diff(const char *extcmd, int symlinks,
 +static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
  			int argc, const char **argv)
  {
  	char tmpdir[PATH_MAX];
 @@ -283,7 +277,6 @@ static int run_dir_diff(const char *extcmd, int symlinks,
  	struct hashmap wt_modified, tmp_modified;
  	int indices_loaded = 0;
  
 -	setup_work_tree();
  	workdir = get_git_work_tree();
  
  	/* Setup temp directories */
 @@ -323,6 +316,7 @@ static int run_dir_diff(const char *extcmd, int symlinks,
  	child.git_cmd = 1;
  	child.use_shell = 0;
  	child.clean_on_exit = 1;
 +	child.dir = prefix;
  	child.out = -1;
  	argv_array_pushl(&child.args, "diff", "--raw", "--no-abbrev", "-z",
  			 NULL);
 @@ -333,6 +327,7 @@ static int run_dir_diff(const char *extcmd, int symlinks,
  	fp = xfdopen(child.out, "r");
  
  	/* Build index info for left and right sides of the diff */
 +	i = 0;
  	while (!strbuf_getline_nul(&info, fp)) {
  		int lmode, rmode;
  		struct object_id loid, roid;
 @@ -353,6 +348,7 @@ static int run_dir_diff(const char *extcmd, int symlinks,
  		src_path = lpath.buf;
  		src_path_len = lpath.len;
  
 +		i++;
  		if (status != 'C' && status != 'R') {
  			dst_path = src_path;
  			dst_path_len = src_path_len;
 @@ -456,11 +452,15 @@ static int run_dir_diff(const char *extcmd, int symlinks,
  			}
  		}
  	}
 +
  	if (finish_command(&child)) {
  		ret = error("error occurred running diff --raw");
  		goto finish;
  	}
  
 +	if (!i)
 +		return 0;
 +
  	/*
  	 * Changes to submodules require special treatment.This loop writes a
  	 * temporary file to both the left and right directories to show the
 @@ -591,7 +591,8 @@ static int run_dir_diff(const char *extcmd, int symlinks,
  	return ret;
  }
  
 -static int run_file_diff(int prompt, int argc, const char **argv)
 +static int run_file_diff(int prompt, const char *prefix,
 +			 int argc, const char **argv)
  {
  	struct argv_array args = ARGV_ARRAY_INIT;
  	const char *env[] = {
 @@ -605,14 +606,39 @@ static int run_file_diff(int prompt, int argc, const char **argv)
  	else if (!prompt)
  		env[2] = "GIT_DIFFTOOL_NO_PROMPT=true";
  
 +
  	argv_array_push(&args, "diff");
  	for (i = 0; i < argc; i++)
  		argv_array_push(&args, argv[i]);
 -	ret = run_command_v_opt_cd_env(args.argv, RUN_GIT_CMD, NULL, env);
 +	ret = run_command_v_opt_cd_env(args.argv, RUN_GIT_CMD, prefix, env);
  	exit(ret);
  }
  
 -int cmd_difftool(int argc, const char ** argv, const char * prefix)
 +/*
 + * NEEDSWORK: this function can go once the legacy-difftool Perl script is
 + * retired.
 + *
 + * We intentionally avoid reading the config directly here, to avoid messing up
 + * the GIT_* environment variables when we need to fall back to exec()ing the
 + * Perl script.
 + */
 +static int use_builtin_difftool(void) {
 +	struct child_process cp = CHILD_PROCESS_INIT;
 +	struct strbuf out = STRBUF_INIT;
 +	int ret;
 +
 +	argv_array_pushl(&cp.args,
 +			 "config", "--bool", "difftool.usebuiltin", NULL);
 +	cp.git_cmd = 1;
 +	if (capture_command(&cp, &out, 6))
 +		return 0;
 +	strbuf_trim(&out);
 +	ret = !strcmp("true", out.buf);
 +	strbuf_release(&out);
 +	return ret;
 +}
 +
 +int cmd_difftool(int argc, const char **argv, const char *prefix)
  {
  	int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,
  	    tool_help = 0;
 @@ -643,16 +669,29 @@ int cmd_difftool(int argc, const char ** argv, const char * prefix)
  		OPT_END()
  	};
  
 -	git_config(difftool_config, NULL);
 -	symlinks = has_symlinks;
 -	if (!use_builtin_difftool) {
 -		const char *path = mkpath("%s/git-legacy-difftool", git_exec_path());
 +	/*
 +	 * NEEDSWORK: Once the builtin difftool has been tested enough
 +	 * and git-legacy-difftool.perl is retired to contrib/, this preamble
 +	 * can be removed.
 +	 */
 +	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;
  	}
 +	prefix = setup_git_directory();
 +	trace_repo_setup(prefix);
 +	setup_work_tree();
 +	/* NEEDSWORK: once we no longer spawn anything, remove this */
 +	setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
 +	setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);
 +
 +	git_config(difftool_config, NULL);
 +	symlinks = has_symlinks;
  
  	argc = parse_options(argc, argv, prefix, builtin_difftool_options,
  			     builtin_difftool_usage, PARSE_OPT_KEEP_UNKNOWN |
 @@ -687,6 +726,6 @@ int cmd_difftool(int argc, const char ** argv, const char * prefix)
  	 * each file that changed.
  	 */
  	if (dir_diff)
 -		return run_dir_diff(extcmd, symlinks, argc, argv);
 -	return run_file_diff(prompt, argc, argv);
 +		return run_dir_diff(extcmd, symlinks, prefix, argc, argv);
 +	return run_file_diff(prompt, prefix, argc, argv);
  }
 diff --git a/git.c b/git.c
 index e68b6eb..a8e6a15 100644
 --- a/git.c
 +++ b/git.c
 @@ -424,7 +424,12 @@ 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 },
 +	/*
 +	 * NEEDSWORK: Once the redirection to git-legacy-difftool.perl in
 +	 * builtin/difftool.c has been removed, this entry should be changed to
 +	 * RUN_SETUP | NEED_WORK_TREE
 +	 */
 +	{ "difftool", cmd_difftool },
  	{ "fast-export", cmd_fast_export, RUN_SETUP },
  	{ "fetch", cmd_fetch, RUN_SETUP },
  	{ "fetch-pack", cmd_fetch_pack, RUN_SETUP },
 diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
 index 70a2de4..b6a6c30 100755
 --- a/t/t7800-difftool.sh
 +++ b/t/t7800-difftool.sh
 @@ -23,6 +23,8 @@ prompt_given ()
  	test "$prompt" = "Launch 'test-tool' [Y/n]? branch"
  }
  
 +# NEEDSWORK: lose all the PERL prereqs once legacy-difftool is retired.
 +
  # Create a file on master and change it on branch
  test_expect_success PERL 'setup' '
  	echo master >file &&

-- 
2.10.1.583.g721a9e0