Re: [PATCH v5 0/3] Turn the difftool into a builtin

[Junio C Hamano]

Johannes Schindelin  writes:

> - replaced the cross-validation with the Perl script by a patch that
>   retires the Perl script instead.

Yup.  That makes things a lot simpler.  While we try to be careful
during major rewrite, we usually do not go extra careful to leave
two competing implementations in-tree.

Will replace js/difftool-builtin topic.  Thanks.

[PATCH v5 0/3] Turn the difftool into a builtin

[Johannes Schindelin]

This patch series converts the difftool from a Perl script into a
builtin, for three 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. As the Perl script uses Unix-y paths that are not native to Windows,
   the Perl interpreter has to go through a POSIX emulation layer (the
   MSYS2 runtime). This means that paths have to be converted from
   Unix-y paths to Windows-y paths (and vice versa) whenever crossing
   the POSIX emulation barrier, leading to quite possibly surprising path
   translation errors.

3. 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 v4:

- skipped the unrelated Coverity-appeasing patch.

- replaced the cross-validation with the Perl script by a patch that
  retires the Perl script instead.


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

 Makefile   |   2 +-
 builtin.h  |   1 +
 builtin/difftool.c | 692 +
 .../examples/git-difftool.perl |   0
 git.c  |   1 +
 t/t7800-difftool.sh|  92 +--
 6 files changed, 741 insertions(+), 47 deletions(-)
 create mode 100644 builtin/difftool.c
 rename git-difftool.perl => contrib/examples/git-difftool.perl (100%)


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

Interdiff vs v4:

 diff --git a/.gitignore b/.gitignore
 index ae025b..6722f78f9a 100644
 --- a/.gitignore
 +++ b/.gitignore
 @@ -76,7 +76,6 @@
  /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 8cf5bef034..e9aa6ae57c 100644
 --- a/Makefile
 +++ b/Makefile
 @@ -522,7 +522,6 @@ SCRIPT_LIB += git-sh-setup
  SCRIPT_LIB += git-sh-i18n
  
  SCRIPT_PERL += git-add--interactive.perl
 -SCRIPT_PERL += git-legacy-difftool.perl
  SCRIPT_PERL += git-archimport.perl
  SCRIPT_PERL += git-cvsexportcommit.perl
  SCRIPT_PERL += git-cvsimport.perl
 diff --git a/builtin/difftool.c b/builtin/difftool.c
 index 2115e548a5..42ad9e804a 100644
 --- a/builtin/difftool.c
 +++ b/builtin/difftool.c
 @@ -616,30 +616,6 @@ static int run_file_diff(int prompt, const char *prefix,
exit(ret);
  }
  
 -/*
 - * 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(,
 -   "config", "--bool", "difftool.usebuiltin", NULL);
 -  cp.git_cmd = 1;
 -  if (capture_command(, , 6))
 -  return 0;
 -  strbuf_trim();
 -  ret = !strcmp("true", out.buf);
 -  strbuf_release();
 -  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,
 @@ -671,23 +647,6 @@ int cmd_difftool(int argc, const char **argv, const char 
*prefix)
OPT_END()
};
  
 -  /*
 -   * 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,