[PATCH v4 0/4] Show Git Mailing List: a builtin difftool

I have been working on the builtin difftool for a while now, 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.

Changes since v3:

- made path_entry_cmp static

- fixed a few bugs identified by Coverity

- fixed overzealous status parsing that did not expect any number after
  C or R

Johannes Schindelin (4):
  Avoid Coverity warning about unfree()d git_exec_path()
  difftool: add a skeleton for the upcoming builtin
  difftool: implement the functionality in the builtin
  t7800: run both builtin and scripted difftool, for now

 .gitignore                                    |   1 +
 Makefile                                      |   3 +-
 builtin.h                                     |   1 +
 builtin/difftool.c                            | 733 ++++++++++++++++++++++++++
 exec_cmd.c                                    |   5 +-
 git-difftool.perl => git-legacy-difftool.perl |   0
 git.c                                         |   6 +
 t/t7800-difftool.sh                           |  29 +
 8 files changed, 776 insertions(+), 2 deletions(-)
 create mode 100644 builtin/difftool.c
 rename git-difftool.perl => git-legacy-difftool.perl (100%)

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

Interdiff vs v3:

 diff --git a/builtin/difftool.c b/builtin/difftool.c
 index 3480920fea..2115e548a5 100644
 --- a/builtin/difftool.c
 +++ b/builtin/difftool.c
 @@ -73,8 +73,10 @@ static int parse_index_info(char *p, int *mode1, int *mode2,
  	if (*p != ' ')
  		return error("expected ' ', got '%c'", *p);
  	*status = *++p;
 -	if (!status || p[1])
 -		return error("unexpected trailer: '%s'", p);
 +	if (!*status)
 +		return error("missing status");
 +	if (p[1] && !isdigit(p[1]))
 +		return error("unexpected trailer: '%s'", p + 1);
  	return 0;
  }
  
 @@ -107,7 +109,8 @@ static int use_wt_file(const char *workdir, const char *name,
  		struct object_id wt_oid;
  		int fd = open(buf.buf, O_RDONLY);
  
 -		if (!index_fd(wt_oid.hash, fd, &st, OBJ_BLOB, name, 0)) {
 +		if (fd >= 0 &&
 +		    !index_fd(wt_oid.hash, fd, &st, OBJ_BLOB, name, 0)) {
  			if (is_null_oid(oid)) {
  				oidcpy(oid, &wt_oid);
  				use = 1;
 @@ -162,7 +165,7 @@ static void add_left_or_right(struct hashmap *map, const char *path,
  		e->left[0] = e->right[0] = '\0';
  		hashmap_add(map, e);
  	}
 -	strcpy(is_right ? e->right : e->left, content);
 +	strlcpy(is_right ? e->right : e->left, content, PATH_MAX);
  }
  
  struct path_entry {
 @@ -170,7 +173,7 @@ struct path_entry {
  	char path[FLEX_ARRAY];
  };
  
 -int path_entry_cmp(struct path_entry *a, struct path_entry *b, void *key)
 +static int path_entry_cmp(struct path_entry *a, struct path_entry *b, void *key)
  {
  	return strcmp(a->path, key ? key : b->path);
  }
 @@ -423,17 +426,16 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
  				struct cache_entry *ce2 =
  					make_cache_entry(rmode, roid.hash,
  							 dst_path, 0, 0);
 -				ce_mode_from_stat(ce2, rmode);
  
  				add_index_entry(&wtindex, ce2,
  						ADD_CACHE_JUST_APPEND);
  
 -				add_path(&wtdir, wtdir_len, dst_path);
  				add_path(&rdir, rdir_len, dst_path);
  				if (ensure_leading_directories(rdir.buf))
  					return error("could not create "
  						     "directory for '%s'",
  						     dst_path);
 +				add_path(&wtdir, wtdir_len, dst_path);
  				if (symlinks) {
  					if (symlink(wtdir.buf, rdir.buf)) {
  						ret = error_errno("could not symlink '%s' to '%s'", wtdir.buf, rdir.buf);
 diff --git a/exec_cmd.c b/exec_cmd.c
 index 19ac2146d0..587bd7eb48 100644
 --- a/exec_cmd.c
 +++ b/exec_cmd.c
 @@ -65,6 +65,7 @@ void git_set_argv_exec_path(const char *exec_path)
  const char *git_exec_path(void)
  {
  	const char *env;
 +	static char *system_exec_path;
  
  	if (argv_exec_path)
  		return argv_exec_path;
 @@ -74,7 +75,9 @@ const char *git_exec_path(void)
  		return env;
  	}
  
 -	return system_path(GIT_EXEC_PATH);
 +	if (!system_exec_path)
 +		system_exec_path = system_path(GIT_EXEC_PATH);
 +	return system_exec_path;
  }
  
  static void add_path(struct strbuf *out, const char *path)
 diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
 index e94910c563..273ab55723 100755
 --- a/t/t7800-difftool.sh
 +++ b/t/t7800-difftool.sh
 @@ -23,6 +23,20 @@ prompt_given ()
  	test "$prompt" = "Launch 'test-tool' [Y/n]? branch"
  }
  
 +for use_builtin_difftool in false true
 +do
 +
 +test_expect_success 'verify we are running the correct difftool' '
 +	if test true = '$use_builtin_difftool'
 +	then
 +		test_must_fail ok=129 git difftool -h >help &&
 +		grep "g, --gui" help
 +	else
 +		git difftool -h >help &&
 +		grep "g|--gui" help
 +	fi
 +'
 +
  # NEEDSWORK: lose all the PERL prereqs once legacy-difftool is retired.
  
  # Create a file on master and change it on branch
 @@ -606,4 +620,17 @@ test_expect_success PERL,SYMLINKS 'difftool --dir-diff symlinked directories' '
  	)
  '
  
 +test true != $use_builtin_difftool || break
 +
 +test_expect_success 'tear down for re-run' '
 +	rm -rf * .[a-z]* &&
 +	git init
 +'
 +
 +# run as builtin difftool now
 +GIT_CONFIG_PARAMETERS="'difftool.usebuiltin=true'"
 +export GIT_CONFIG_PARAMETERS
 +
 +done
 +
  test_done