[PATCH v2 0/1] Show Git Mailing List: a builtin difftool

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 v1:

- fixed the usage (pointed out by David Aguilar)

- moved the stray #include "dir.h" to cuddle with the other #include's
  (also pointed out by David Aguilar)

- changed the `sprintf()` call to a much safer `xsnprintf()` call
  (again, pointed out by David Aguilar)

- changed an error message to include the offending path (need I say,
  pointed out by David Aguilar?)

- used more restrictive permissions for the temporary directories (once
  again, David Aguilar's suggestion)

- fixed a comment that lacked a space after a period (another fix thanks
  to David Aguilar).

- switched the opt-in feature flag triggering the use of the builtin
  difftool to a config variable (this suggestion came from Junio
  Hamano).

- made difftool respect core.symlinks by moving the usage of
  has_symlinks after the config was parsed.

Johannes Schindelin (1):
  difftool: add the builtin

 .gitignore                                    |   1 +
 Makefile                                      |   3 +-
 builtin.h                                     |   1 +
 builtin/difftool.c                            | 692 ++++++++++++++++++++++++++
 git-difftool.perl => git-legacy-difftool.perl |   0
 git.c                                         |   1 +
 6 files changed, 697 insertions(+), 1 deletion(-)
 create mode 100644 builtin/difftool.c
 rename git-difftool.perl => git-legacy-difftool.perl (100%)

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

Interdiff vs v1:

 diff --git a/.gitignore b/.gitignore
 index 91bfd09..f96e50e 100644
 --- a/.gitignore
 +++ b/.gitignore
 @@ -1,4 +1,3 @@
 -/use-builtin-difftool
  /GIT-BUILD-OPTIONS
  /GIT-CFLAGS
  /GIT-LDFLAGS
 @@ -52,7 +51,6 @@
  /git-diff-tree
  /git-difftool
  /git-difftool--helper
 -/git-builtin-difftool
  /git-describe
  /git-fast-export
  /git-fast-import
 @@ -78,6 +76,7 @@
  /git-init-db
  /git-interpret-trailers
  /git-instaweb
 +/git-legacy-difftool
  /git-log
  /git-ls-files
  /git-ls-remote
 diff --git a/Makefile b/Makefile
 index f764174..7863bc2 100644
 --- a/Makefile
 +++ b/Makefile
 @@ -527,7 +527,7 @@ SCRIPT_LIB += git-sh-setup
  SCRIPT_LIB += git-sh-i18n
  
  SCRIPT_PERL += git-add--interactive.perl
 -SCRIPT_PERL += git-difftool.perl
 +SCRIPT_PERL += git-legacy-difftool.perl
  SCRIPT_PERL += git-archimport.perl
  SCRIPT_PERL += git-cvsexportcommit.perl
  SCRIPT_PERL += git-cvsimport.perl
 @@ -888,7 +888,7 @@ BUILTIN_OBJS += builtin/diff-files.o
  BUILTIN_OBJS += builtin/diff-index.o
  BUILTIN_OBJS += builtin/diff-tree.o
  BUILTIN_OBJS += builtin/diff.o
 -BUILTIN_OBJS += builtin/builtin-difftool.o
 +BUILTIN_OBJS += builtin/difftool.o
  BUILTIN_OBJS += builtin/fast-export.o
  BUILTIN_OBJS += builtin/fetch-pack.o
  BUILTIN_OBJS += builtin/fetch.o
 diff --git a/builtin.h b/builtin.h
 index 409a61e..67f8051 100644
 --- a/builtin.h
 +++ b/builtin.h
 @@ -60,7 +60,7 @@ extern int cmd_diff_files(int argc, const char **argv, const char *prefix);
  extern int cmd_diff_index(int argc, const char **argv, const char *prefix);
  extern int cmd_diff(int argc, const char **argv, const char *prefix);
  extern int cmd_diff_tree(int argc, const char **argv, const char *prefix);
 -extern int cmd_builtin_difftool(int argc, const char **argv, const char *prefix);
 +extern int cmd_difftool(int argc, const char **argv, const char *prefix);
  extern int cmd_fast_export(int argc, const char **argv, const char *prefix);
  extern int cmd_fetch(int argc, const char **argv, const char *prefix);
  extern int cmd_fetch_pack(int argc, const char **argv, const char *prefix);
 diff --git a/builtin/builtin-difftool.c b/builtin/difftool.c
 similarity index 95%
 rename from builtin/builtin-difftool.c
 rename to builtin/difftool.c
 index 9feefcd..f845879 100644
 --- a/builtin/builtin-difftool.c
 +++ b/builtin/difftool.c
 @@ -18,12 +18,15 @@
  #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 add [<options>] [--] <pathspec>..."),
 +static const char *const builtin_difftool_usage[] = {
 +	N_("git difftool [<options>] [<commit> [<commit>]] [--] [<path>...]"),
  	NULL
  };
  
 @@ -39,6 +42,11 @@ 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);
  }
  
 @@ -227,8 +235,6 @@ static void changed_files(struct hashmap *result, const char *index_path,
  	strbuf_release(&buf);
  }
  
 -#include "dir.h"
 -
  static NORETURN void exit_cleanup(const char *tmpdir, int exit_code)
  {
  	struct strbuf buf = STRBUF_INIT;
 @@ -282,16 +288,16 @@ static int run_dir_diff(const char *extcmd, int symlinks,
  
  	/* Setup temp directories */
  	tmp = getenv("TMPDIR");
 -	sprintf(tmpdir, "%s/git-difftool.XXXXXX", tmp ? tmp : "/tmp");
 +	xsnprintf(tmpdir, sizeof(tmpdir), "%s/git-difftool.XXXXXX", tmp ? tmp : "/tmp");
  	if (!mkdtemp(tmpdir))
 -		return error("could not create temporary directory");
 +		return error("could not create '%s'", tmpdir);
  	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);
 +	mkdir(ldir.buf, 0700);
 +	mkdir(rdir.buf, 0700);
  
  	memset(&wtindex, 0, sizeof(wtindex));
  
 @@ -606,12 +612,11 @@ static int run_file_diff(int prompt, int argc, const char **argv)
  	exit(ret);
  }
  
 -int cmd_builtin_difftool(int argc, const char ** argv, const char * prefix)
 +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;
  	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`")),
 @@ -638,9 +643,16 @@ int cmd_builtin_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());
  
 -	git_config(difftool_config, NULL);
 +		if (sane_execvp(path, (char **)argv) < 0)
 +			die_errno("could not exec %s", path);
 +
 +		return 0;
 +	}
  
  	argc = parse_options(argc, argv, prefix, builtin_difftool_options,
  			     builtin_difftool_usage, PARSE_OPT_KEEP_UNKNOWN |
 @@ -670,7 +682,7 @@ int cmd_builtin_difftool(int argc, const char ** argv, const char * prefix)
  
  	/*
  	 * In directory diff mode, 'git-difftool--helper' is called once
 -	 * to compare the a / b directories.In file diff mode, 'git diff'
 +	 * 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.
  	 */
 diff --git a/git-difftool.perl b/git-legacy-difftool.perl
 similarity index 98%
 rename from git-difftool.perl
 rename to git-legacy-difftool.perl
 index 28e47d8..a5790d0 100755
 --- a/git-difftool.perl
 +++ b/git-legacy-difftool.perl
 @@ -23,13 +23,6 @@ use File::Temp qw(tempdir);
  use Getopt::Long qw(:config pass_through);
  use Git;
  
 -if (-e Git::exec_path() . '/use-builtin-difftool') {
 -	unshift(@ARGV, "builtin-difftool");
 -	unshift(@ARGV, "git");
 -	exec(@ARGV);
 -	die("Could not execute builtin difftool");
 -}
 -
  sub usage
  {
  	my $exitcode = shift;
 diff --git a/git.c b/git.c
 index 7a0df7a..0e6bbee 100644
 --- a/git.c
 +++ b/git.c
 @@ -2,7 +2,6 @@
  #include "exec_cmd.h"
  #include "help.h"
  #include "run-command.h"
 -#include "dir.h"
  
  const char git_usage_string[] =
  	"git [--version] [--help] [-C <path>] [-c name=value]\n"
 @@ -425,7 +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 },
 +	{ "difftool", cmd_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 },
 @@ -543,22 +542,6 @@ static void strip_extension(const char **argv)
  #define strip_extension(cmd)
  #endif
  
 -static int use_builtin_difftool(void)
 -{
 -	static int initialized, use;
 -
 -	if (!initialized) {
 -		struct strbuf buf = STRBUF_INIT;
 -		strbuf_addf(&buf, "%s/%s", git_exec_path(),
 -			    "use-builtin-difftool");
 -		use = file_exists(buf.buf);
 -		strbuf_release(&buf);
 -		initialized = 1;
 -	}
 -
 -	return use;
 -}
 -
  static void handle_builtin(int argc, const char **argv)
  {
  	struct argv_array args = ARGV_ARRAY_INIT;
 @@ -568,9 +551,6 @@ static void handle_builtin(int argc, const char **argv)
  	strip_extension(argv);
  	cmd = argv[0];
  
 -	if (!strcmp("difftool", cmd) && use_builtin_difftool())
 -		cmd = "builtin-difftool";
 -
  	/* Turn "git cmd --help" into "git help --exclude-guides cmd" */
  	if (argc > 1 && !strcmp(argv[1], "--help")) {
  		int i;