From 18a25ef645709b6c3f44bea10ed5ce4d9cf7b1c1 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 17 Jan 2017 16:54:57 +0100 Subject: [PATCH 1/7] difftool: add a skeleton for the upcoming builtin This adds a builtin difftool that still falls back to the legacy Perl version, which has been renamed to `legacy-difftool`. The idea is that the new, experimental, builtin difftool immediately hands off to the legacy difftool for now, unless the config variable difftool.useBuiltin is set to true. This feature flag will be used in the upcoming Git for Windows v2.11.0 release, to allow early testers to opt-in to use the builtin difftool and flesh out any bugs. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- .gitignore | 1 + Makefile | 3 +- builtin.h | 1 + builtin/difftool.c | 63 +++++++++++++++++++ git-difftool.perl => git-legacy-difftool.perl | 0 git.c | 6 ++ t/t7800-difftool.sh | 2 + 7 files changed, 75 insertions(+), 1 deletion(-) create mode 100644 builtin/difftool.c rename git-difftool.perl => git-legacy-difftool.perl (100%) diff --git a/.gitignore b/.gitignore index 6722f78f9a..5555ae025b 100644 --- a/.gitignore +++ b/.gitignore @@ -76,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 76267262c1..4137a9915b 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,6 +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/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 b9122bc5f4..67f80519da 100644 --- a/builtin.h +++ b/builtin.h @@ -60,6 +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_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/difftool.c b/builtin/difftool.c new file mode 100644 index 0000000000..53870bbaf7 --- /dev/null +++ b/builtin/difftool.c @@ -0,0 +1,63 @@ +/* + * "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 "builtin.h" +#include "run-command.h" +#include "exec_cmd.h" + +/* + * 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) +{ + /* + * 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(); + + die("TODO"); +} diff --git a/git-difftool.perl b/git-legacy-difftool.perl similarity index 100% rename from git-difftool.perl rename to git-legacy-difftool.perl diff --git a/git.c b/git.c index c8fe6637df..519a4c6f82 100644 --- a/git.c +++ b/git.c @@ -424,6 +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 }, + /* + * 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 99d4123461..e94910c563 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 && From e98aec3e23043f66bc5d2359b1628ab0d5d3d184 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 19 Jan 2017 21:30:36 +0100 Subject: [PATCH 2/7] difftool: implement the functionality in the builtin This patch gives life to the skeleton added in the previous patch. The motivation for converting the difftool 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, and it will often need to perform unnecessary work (e.g. when reading the entire config every time `git config` is called to query a single config value). 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 later. Note: to play it safe, the original difftool is still called unless the config setting difftool.useBuiltin is set to true. The reason: this new, experimental, builtin difftool was shipped as part of Git for Windows v2.11.0, to allow for easier large-scale testing, but of course as an opt-in feature. The speedup is actually more noticable on Linux than on Windows: a quick test shows that t7800-difftool.sh runs in (2.183s/0.052s/0.108s) (real/user/sys) in a Linux VM, down from (6.529s/3.112s/0.644s), while on Windows, it is (36.064s/2.730s/7.194s), down from (47.637s/2.407s/6.863s). The culprit is most likely the overhead incurred from *still* having to shell out to mergetool-lib.sh and difftool--helper.sh. Still, it is an improvement. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- builtin/difftool.c | 672 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 671 insertions(+), 1 deletion(-) diff --git a/builtin/difftool.c b/builtin/difftool.c index 53870bbaf7..2115e548a5 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -11,9 +11,610 @@ * * Copyright (C) 2016 Johannes Schindelin */ +#include "cache.h" #include "builtin.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" + +static char *diff_gui_tool; +static int trust_exit_code; + +static const char *const builtin_difftool_usage[] = { + N_("git difftool [] [ []] [--] [...]"), + NULL +}; + +static int difftool_config(const char *var, const char *value, void *cb) +{ + if (!strcmp(var, "diff.guitool")) { + diff_gui_tool = xstrdup(value); + return 0; + } + + if (!strcmp(var, "difftool.trustexitcode")) { + trust_exit_code = git_config_bool(var, value); + return 0; + } + + return git_default_config(var, value, cb); +} + +static int print_tool_help(void) +{ + const char *argv[] = { "mergetool", "--tool-help=diff", NULL }; + return run_command_v_opt(argv, RUN_GIT_CMD); +} + +static int parse_index_info(char *p, int *mode1, int *mode2, + struct object_id *oid1, struct object_id *oid2, + char *status) +{ + if (*p != ':') + return error("expected ':', got '%c'", *p); + *mode1 = (int)strtol(p + 1, &p, 8); + if (*p != ' ') + return error("expected ' ', got '%c'", *p); + *mode2 = (int)strtol(p + 1, &p, 8); + if (*p != ' ') + return error("expected ' ', got '%c'", *p); + if (get_oid_hex(++p, oid1)) + return error("expected object ID, got '%s'", p + 1); + p += GIT_SHA1_HEXSZ; + if (*p != ' ') + return error("expected ' ', got '%c'", *p); + if (get_oid_hex(++p, oid2)) + return error("expected object ID, got '%s'", p + 1); + p += GIT_SHA1_HEXSZ; + if (*p != ' ') + return error("expected ' ', got '%c'", *p); + *status = *++p; + if (!*status) + return error("missing status"); + if (p[1] && !isdigit(p[1])) + return error("unexpected trailer: '%s'", p + 1); + return 0; +} + +/* + * Remove any trailing slash from $workdir + * before starting to avoid double slashes in symlink targets. + */ +static void add_path(struct strbuf *buf, size_t base_len, const char *path) +{ + strbuf_setlen(buf, base_len); + if (buf->len && buf->buf[buf->len - 1] != '/') + strbuf_addch(buf, '/'); + strbuf_addstr(buf, path); +} + +/* + * Determine whether we can simply reuse the file in the worktree. + */ +static int use_wt_file(const char *workdir, const char *name, + struct object_id *oid) +{ + struct strbuf buf = STRBUF_INIT; + struct stat st; + int use = 0; + + strbuf_addstr(&buf, workdir); + add_path(&buf, buf.len, name); + + if (!lstat(buf.buf, &st) && !S_ISLNK(st.st_mode)) { + struct object_id wt_oid; + int fd = open(buf.buf, O_RDONLY); + + 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; + } else if (!oidcmp(oid, &wt_oid)) + use = 1; + } + } + + strbuf_release(&buf); + + return use; +} + +struct working_tree_entry { + struct hashmap_entry entry; + char path[FLEX_ARRAY]; +}; + +static int working_tree_entry_cmp(struct working_tree_entry *a, + struct working_tree_entry *b, void *keydata) +{ + return strcmp(a->path, b->path); +} + +/* + * The `left` and `right` entries hold paths for the symlinks hashmap, + * and a SHA-1 surrounded by brief text for submodules. + */ +struct pair_entry { + struct hashmap_entry entry; + char left[PATH_MAX], right[PATH_MAX]; + const char path[FLEX_ARRAY]; +}; + +static int pair_cmp(struct pair_entry *a, struct pair_entry *b, void *keydata) +{ + return strcmp(a->path, b->path); +} + +static void add_left_or_right(struct hashmap *map, const char *path, + const char *content, int is_right) +{ + struct pair_entry *e, *existing; + + FLEX_ALLOC_STR(e, path, path); + hashmap_entry_init(e, strhash(path)); + existing = hashmap_get(map, e, NULL); + if (existing) { + free(e); + e = existing; + } else { + e->left[0] = e->right[0] = '\0'; + hashmap_add(map, e); + } + strlcpy(is_right ? e->right : e->left, content, PATH_MAX); +} + +struct path_entry { + struct hashmap_entry entry; + char path[FLEX_ARRAY]; +}; + +static int path_entry_cmp(struct path_entry *a, struct path_entry *b, void *key) +{ + return strcmp(a->path, key ? key : b->path); +} + +static void changed_files(struct hashmap *result, const char *index_path, + const char *workdir) +{ + struct child_process update_index = CHILD_PROCESS_INIT; + struct child_process diff_files = CHILD_PROCESS_INIT; + struct strbuf index_env = STRBUF_INIT, buf = STRBUF_INIT; + const char *git_dir = absolute_path(get_git_dir()), *env[] = { + NULL, NULL + }; + FILE *fp; + + strbuf_addf(&index_env, "GIT_INDEX_FILE=%s", index_path); + env[0] = index_env.buf; + + argv_array_pushl(&update_index.args, + "--git-dir", git_dir, "--work-tree", workdir, + "update-index", "--really-refresh", "-q", + "--unmerged", NULL); + update_index.no_stdin = 1; + update_index.no_stdout = 1; + update_index.no_stderr = 1; + update_index.git_cmd = 1; + update_index.use_shell = 0; + update_index.clean_on_exit = 1; + update_index.dir = workdir; + update_index.env = env; + /* Ignore any errors of update-index */ + run_command(&update_index); + + argv_array_pushl(&diff_files.args, + "--git-dir", git_dir, "--work-tree", workdir, + "diff-files", "--name-only", "-z", NULL); + diff_files.no_stdin = 1; + diff_files.git_cmd = 1; + diff_files.use_shell = 0; + diff_files.clean_on_exit = 1; + diff_files.out = -1; + diff_files.dir = workdir; + diff_files.env = env; + if (start_command(&diff_files)) + die("could not obtain raw diff"); + fp = xfdopen(diff_files.out, "r"); + while (!strbuf_getline_nul(&buf, fp)) { + struct path_entry *entry; + FLEX_ALLOC_STR(entry, path, buf.buf); + hashmap_entry_init(entry, strhash(buf.buf)); + hashmap_add(result, entry); + } + if (finish_command(&diff_files)) + die("diff-files did not exit properly"); + strbuf_release(&index_env); + strbuf_release(&buf); +} + +static NORETURN void exit_cleanup(const char *tmpdir, int exit_code) +{ + struct strbuf buf = STRBUF_INIT; + strbuf_addstr(&buf, tmpdir); + remove_dir_recursively(&buf, 0); + if (exit_code) + warning(_("failed: %d"), exit_code); + exit(exit_code); +} + +static int ensure_leading_directories(char *path) +{ + switch (safe_create_leading_directories(path)) { + case SCLD_OK: + case SCLD_EXISTS: + return 0; + default: + return error(_("could not create leading directories " + "of '%s'"), path); + } +} + +static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, + 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; + + workdir = get_git_work_tree(); + + /* Setup temp directories */ + tmp = getenv("TMPDIR"); + xsnprintf(tmpdir, sizeof(tmpdir), "%s/git-difftool.XXXXXX", tmp ? tmp : "/tmp"); + if (!mkdtemp(tmpdir)) + 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, 0700); + mkdir(rdir.buf, 0700); + + memset(&wtindex, 0, sizeof(wtindex)); + + memset(&lstate, 0, sizeof(lstate)); + lstate.base_dir = ldir.buf; + lstate.base_dir_len = ldir.len; + lstate.force = 1; + memset(&rstate, 0, sizeof(rstate)); + rstate.base_dir = rdir.buf; + rstate.base_dir_len = rdir.len; + rstate.force = 1; + + ldir_len = ldir.len; + rdir_len = rdir.len; + wtdir_len = wtdir.len; + + hashmap_init(&working_tree_dups, + (hashmap_cmp_fn)working_tree_entry_cmp, 0); + hashmap_init(&submodules, (hashmap_cmp_fn)pair_cmp, 0); + hashmap_init(&symlinks2, (hashmap_cmp_fn)pair_cmp, 0); + + child.no_stdin = 1; + 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); + for (i = 0; i < argc; i++) + argv_array_push(&child.args, argv[i]); + if (start_command(&child)) + die("could not obtain raw diff"); + 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; + char status; + const char *src_path, *dst_path; + size_t src_path_len, dst_path_len; + + if (starts_with(info.buf, "::")) + die(N_("combined diff formats('-c' and '--cc') are " + "not supported in\n" + "directory diff mode('-d' and '--dir-diff').")); + + if (parse_index_info(info.buf, &lmode, &rmode, &loid, &roid, + &status)) + break; + if (strbuf_getline_nul(&lpath, fp)) + break; + 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; + } else { + if (strbuf_getline_nul(&rpath, fp)) + break; + dst_path = rpath.buf; + dst_path_len = rpath.len; + } + + if (S_ISGITLINK(lmode) || S_ISGITLINK(rmode)) { + strbuf_reset(&buf); + strbuf_addf(&buf, "Subproject commit %s", + oid_to_hex(&loid)); + add_left_or_right(&submodules, src_path, buf.buf, 0); + strbuf_reset(&buf); + strbuf_addf(&buf, "Subproject commit %s", + oid_to_hex(&roid)); + if (!oidcmp(&loid, &roid)) + strbuf_addstr(&buf, "-dirty"); + add_left_or_right(&submodules, dst_path, buf.buf, 1); + continue; + } + + if (S_ISLNK(lmode)) { + char *content = read_sha1_file(loid.hash, &type, &size); + add_left_or_right(&symlinks2, src_path, content, 0); + free(content); + } + + if (S_ISLNK(rmode)) { + char *content = read_sha1_file(roid.hash, &type, &size); + add_left_or_right(&symlinks2, dst_path, content, 1); + free(content); + } + + if (lmode && status != 'C') { + ce->ce_mode = lmode; + oidcpy(&ce->oid, &loid); + strcpy(ce->name, src_path); + ce->ce_namelen = src_path_len; + if (checkout_entry(ce, &lstate, NULL)) + return error("could not write '%s'", src_path); + } + + if (rmode) { + struct working_tree_entry *entry; + + /* Avoid duplicate working_tree entries */ + FLEX_ALLOC_STR(entry, path, dst_path); + hashmap_entry_init(entry, strhash(dst_path)); + if (hashmap_get(&working_tree_dups, entry, NULL)) { + free(entry); + continue; + } + hashmap_add(&working_tree_dups, entry); + + if (!use_wt_file(workdir, dst_path, &roid)) { + ce->ce_mode = rmode; + oidcpy(&ce->oid, &roid); + strcpy(ce->name, dst_path); + ce->ce_namelen = dst_path_len; + if (checkout_entry(ce, &rstate, NULL)) + return error("could not write '%s'", + dst_path); + } else if (!is_null_oid(&roid)) { + /* + * Changes in the working tree need special + * treatment since they are not part of the + * index. + */ + struct cache_entry *ce2 = + make_cache_entry(rmode, roid.hash, + dst_path, 0, 0); + + add_index_entry(&wtindex, ce2, + ADD_CACHE_JUST_APPEND); + + 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); + goto finish; + } + } else { + struct stat st; + if (stat(wtdir.buf, &st)) + st.st_mode = 0644; + if (copy_file(rdir.buf, wtdir.buf, + st.st_mode)) { + ret = error("could not copy '%s' to '%s'", wtdir.buf, rdir.buf); + goto finish; + } + } + } + } + } + + 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 + * change in the recorded SHA1 for the submodule. + */ + hashmap_iter_init(&submodules, &iter); + while ((entry = hashmap_iter_next(&iter))) { + if (*entry->left) { + add_path(&ldir, ldir_len, entry->path); + ensure_leading_directories(ldir.buf); + write_file(ldir.buf, "%s", entry->left); + } + if (*entry->right) { + add_path(&rdir, rdir_len, entry->path); + ensure_leading_directories(rdir.buf); + write_file(rdir.buf, "%s", entry->right); + } + } + + /* + * Symbolic links require special treatment.The standard "git diff" + * shows only the link itself, not the contents of the link target. + * This loop replicates that behavior. + */ + hashmap_iter_init(&symlinks2, &iter); + while ((entry = hashmap_iter_next(&iter))) { + if (*entry->left) { + add_path(&ldir, ldir_len, entry->path); + ensure_leading_directories(ldir.buf); + write_file(ldir.buf, "%s", entry->left); + } + if (*entry->right) { + add_path(&rdir, rdir_len, entry->path); + ensure_leading_directories(rdir.buf); + write_file(rdir.buf, "%s", entry->right); + } + } + + strbuf_release(&buf); + + strbuf_setlen(&ldir, ldir_len); + helper_argv[1] = ldir.buf; + strbuf_setlen(&rdir, rdir_len); + helper_argv[2] = rdir.buf; + + if (extcmd) { + helper_argv[0] = extcmd; + flags = 0; + } else + setenv("GIT_DIFFTOOL_DIRDIFF", "true", 1); + rc = run_command_v_opt(helper_argv, flags); + + /* + * If the diff includes working copy files and those + * files were modified during the diff, then the changes + * should be copied back to the working tree. + * Do not copy back files when symlinks are used and the + * external tool did not replace the original link with a file. + * + * These hashes are loaded lazily since they aren't needed + * in the common case of --symlinks and the difftool updating + * files through the symlink. + */ + hashmap_init(&wt_modified, (hashmap_cmp_fn)path_entry_cmp, + wtindex.cache_nr); + hashmap_init(&tmp_modified, (hashmap_cmp_fn)path_entry_cmp, + wtindex.cache_nr); + + for (i = 0; i < wtindex.cache_nr; i++) { + struct hashmap_entry dummy; + const char *name = wtindex.cache[i]->name; + struct stat st; + + add_path(&rdir, rdir_len, name); + if (lstat(rdir.buf, &st)) + continue; + + if ((symlinks && S_ISLNK(st.st_mode)) || !S_ISREG(st.st_mode)) + continue; + + if (!indices_loaded) { + static struct lock_file lock; + strbuf_reset(&buf); + strbuf_addf(&buf, "%s/wtindex", tmpdir); + if (hold_lock_file_for_update(&lock, buf.buf, 0) < 0 || + write_locked_index(&wtindex, &lock, COMMIT_LOCK)) { + ret = error("could not write %s", buf.buf); + rollback_lock_file(&lock); + goto finish; + } + changed_files(&wt_modified, buf.buf, workdir); + strbuf_setlen(&rdir, rdir_len); + changed_files(&tmp_modified, buf.buf, rdir.buf); + add_path(&rdir, rdir_len, name); + indices_loaded = 1; + } + + hashmap_entry_init(&dummy, strhash(name)); + if (hashmap_get(&tmp_modified, &dummy, name)) { + add_path(&wtdir, wtdir_len, name); + if (hashmap_get(&wt_modified, &dummy, name)) { + warning(_("both files modified: '%s' and '%s'."), + wtdir.buf, rdir.buf); + warning(_("working tree file has been left.")); + warning(""); + err = 1; + } else if (unlink(wtdir.buf) || + copy_file(wtdir.buf, rdir.buf, st.st_mode)) + warning_errno(_("could not copy '%s' to '%s'"), + rdir.buf, wtdir.buf); + } + } + + if (err) { + warning(_("temporary files exist in '%s'."), tmpdir); + warning(_("you may want to cleanup or recover these.")); + exit(1); + } else + exit_cleanup(tmpdir, rc); + +finish: + free(ce); + strbuf_release(&ldir); + strbuf_release(&rdir); + strbuf_release(&wtdir); + strbuf_release(&buf); + + return ret; +} + +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[] = { + "GIT_PAGER=", "GIT_EXTERNAL_DIFF=git-difftool--helper", NULL, + NULL + }; + int ret = 0, i; + + if (prompt > 0) + env[2] = "GIT_DIFFTOOL_PROMPT=true"; + 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, prefix, env); + exit(ret); +} /* * NEEDSWORK: this function can go once the legacy-difftool Perl script is @@ -41,6 +642,35 @@ static int use_builtin_difftool(void) { 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`")), + 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_(""), + 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_(""), + N_("specify a custom command for viewing diffs")), + OPT_END() + }; + /* * NEEDSWORK: Once the builtin difftool has been tested enough * and git-legacy-difftool.perl is retired to contrib/, this preamble @@ -58,6 +688,46 @@ int cmd_difftool(int argc, const char **argv, const char *prefix) 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); - die("TODO"); + git_config(difftool_config, NULL); + symlinks = has_symlinks; + + 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 given for --tool=")); + } + + if (extcmd) { + if (*extcmd) + setenv("GIT_DIFFTOOL_EXTCMD", extcmd, 1); + else + die(_("no given for --extcmd=")); + } + + 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. + */ + if (dir_diff) + return run_dir_diff(extcmd, symlinks, prefix, argc, argv); + return run_file_diff(prompt, prefix, argc, argv); } From 4168eb85e307db4b5e3bdb269d81286b70336edb Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 19 Jan 2017 21:30:40 +0100 Subject: [PATCH 3/7] difftool: retire the scripted version It served its purpose, but now we have a builtin difftool. Time for the Perl script to enjoy Florida. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- .gitignore | 1 - Makefile | 1 - builtin/difftool.c | 41 -------- .../examples/git-difftool.perl | 0 git.c | 7 +- t/t7800-difftool.sh | 94 +++++++++---------- 6 files changed, 47 insertions(+), 97 deletions(-) rename git-legacy-difftool.perl => contrib/examples/git-difftool.perl (100%) diff --git a/.gitignore b/.gitignore index 5555ae025b..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 4137a9915b..71a4273f1d 100644 --- a/Makefile +++ b/Makefile @@ -527,7 +527,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(&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, @@ -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, absolute_path(get_git_work_tree()), 1); diff --git a/git-legacy-difftool.perl b/contrib/examples/git-difftool.perl similarity index 100% rename from git-legacy-difftool.perl rename to contrib/examples/git-difftool.perl diff --git a/git.c b/git.c index 519a4c6f82..d2d843d824 100644 --- a/git.c +++ b/git.c @@ -424,12 +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 }, - /* - * 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 }, + { "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 }, diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh index e94910c563..aa0ef02597 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh @@ -23,10 +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' ' +test_expect_success 'setup' ' echo master >file && git add file && git commit -m "added file" && @@ -38,7 +36,7 @@ test_expect_success PERL 'setup' ' ' # Configure a custom difftool..cmd and use it -test_expect_success PERL 'custom commands' ' +test_expect_success 'custom commands' ' difftool_test_setup && test_config difftool.test-tool.cmd "cat \"\$REMOTE\"" && echo master >expect && @@ -51,21 +49,21 @@ test_expect_success PERL 'custom commands' ' test_cmp expect actual ' -test_expect_success PERL 'custom tool commands override built-ins' ' +test_expect_success 'custom tool commands override built-ins' ' test_config difftool.vimdiff.cmd "cat \"\$REMOTE\"" && echo master >expect && git difftool --tool vimdiff --no-prompt branch >actual && test_cmp expect actual ' -test_expect_success PERL 'difftool ignores bad --tool values' ' +test_expect_success 'difftool ignores bad --tool values' ' : >expect && test_must_fail \ git difftool --no-prompt --tool=bad-tool branch >actual && test_cmp expect actual ' -test_expect_success PERL 'difftool forwards arguments to diff' ' +test_expect_success 'difftool forwards arguments to diff' ' difftool_test_setup && >for-diff && git add for-diff && @@ -78,40 +76,40 @@ test_expect_success PERL 'difftool forwards arguments to diff' ' rm for-diff ' -test_expect_success PERL 'difftool ignores exit code' ' +test_expect_success 'difftool ignores exit code' ' test_config difftool.error.cmd false && git difftool -y -t error branch ' -test_expect_success PERL 'difftool forwards exit code with --trust-exit-code' ' +test_expect_success 'difftool forwards exit code with --trust-exit-code' ' test_config difftool.error.cmd false && test_must_fail git difftool -y --trust-exit-code -t error branch ' -test_expect_success PERL 'difftool forwards exit code with --trust-exit-code for built-ins' ' +test_expect_success 'difftool forwards exit code with --trust-exit-code for built-ins' ' test_config difftool.vimdiff.path false && test_must_fail git difftool -y --trust-exit-code -t vimdiff branch ' -test_expect_success PERL 'difftool honors difftool.trustExitCode = true' ' +test_expect_success 'difftool honors difftool.trustExitCode = true' ' test_config difftool.error.cmd false && test_config difftool.trustExitCode true && test_must_fail git difftool -y -t error branch ' -test_expect_success PERL 'difftool honors difftool.trustExitCode = false' ' +test_expect_success 'difftool honors difftool.trustExitCode = false' ' test_config difftool.error.cmd false && test_config difftool.trustExitCode false && git difftool -y -t error branch ' -test_expect_success PERL 'difftool ignores exit code with --no-trust-exit-code' ' +test_expect_success 'difftool ignores exit code with --no-trust-exit-code' ' test_config difftool.error.cmd false && test_config difftool.trustExitCode true && git difftool -y --no-trust-exit-code -t error branch ' -test_expect_success PERL 'difftool stops on error with --trust-exit-code' ' +test_expect_success 'difftool stops on error with --trust-exit-code' ' test_when_finished "rm -f for-diff .git/fail-right-file" && test_when_finished "git reset -- for-diff" && write_script .git/fail-right-file <<-\EOF && @@ -126,13 +124,13 @@ test_expect_success PERL 'difftool stops on error with --trust-exit-code' ' test_cmp expect actual ' -test_expect_success PERL 'difftool honors exit status if command not found' ' +test_expect_success 'difftool honors exit status if command not found' ' test_config difftool.nonexistent.cmd i-dont-exist && test_config difftool.trustExitCode false && test_must_fail git difftool -y -t nonexistent branch ' -test_expect_success PERL 'difftool honors --gui' ' +test_expect_success 'difftool honors --gui' ' difftool_test_setup && test_config merge.tool bogus-tool && test_config diff.tool bogus-tool && @@ -143,7 +141,7 @@ test_expect_success PERL 'difftool honors --gui' ' test_cmp expect actual ' -test_expect_success PERL 'difftool --gui last setting wins' ' +test_expect_success 'difftool --gui last setting wins' ' difftool_test_setup && : >expect && git difftool --no-prompt --gui --no-gui >actual && @@ -157,7 +155,7 @@ test_expect_success PERL 'difftool --gui last setting wins' ' test_cmp expect actual ' -test_expect_success PERL 'difftool --gui works without configured diff.guitool' ' +test_expect_success 'difftool --gui works without configured diff.guitool' ' difftool_test_setup && echo branch >expect && git difftool --no-prompt --gui branch >actual && @@ -165,7 +163,7 @@ test_expect_success PERL 'difftool --gui works without configured diff.guitool' ' # Specify the diff tool using $GIT_DIFF_TOOL -test_expect_success PERL 'GIT_DIFF_TOOL variable' ' +test_expect_success 'GIT_DIFF_TOOL variable' ' difftool_test_setup && git config --unset diff.tool && echo branch >expect && @@ -175,7 +173,7 @@ test_expect_success PERL 'GIT_DIFF_TOOL variable' ' # Test the $GIT_*_TOOL variables and ensure # that $GIT_DIFF_TOOL always wins unless --tool is specified -test_expect_success PERL 'GIT_DIFF_TOOL overrides' ' +test_expect_success 'GIT_DIFF_TOOL overrides' ' difftool_test_setup && test_config diff.tool bogus-tool && test_config merge.tool bogus-tool && @@ -193,7 +191,7 @@ test_expect_success PERL 'GIT_DIFF_TOOL overrides' ' # Test that we don't have to pass --no-prompt to difftool # when $GIT_DIFFTOOL_NO_PROMPT is true -test_expect_success PERL 'GIT_DIFFTOOL_NO_PROMPT variable' ' +test_expect_success 'GIT_DIFFTOOL_NO_PROMPT variable' ' difftool_test_setup && echo branch >expect && GIT_DIFFTOOL_NO_PROMPT=true git difftool branch >actual && @@ -202,7 +200,7 @@ test_expect_success PERL 'GIT_DIFFTOOL_NO_PROMPT variable' ' # git-difftool supports the difftool.prompt variable. # Test that GIT_DIFFTOOL_PROMPT can override difftool.prompt = false -test_expect_success PERL 'GIT_DIFFTOOL_PROMPT variable' ' +test_expect_success 'GIT_DIFFTOOL_PROMPT variable' ' difftool_test_setup && test_config difftool.prompt false && echo >input && @@ -212,7 +210,7 @@ test_expect_success PERL 'GIT_DIFFTOOL_PROMPT variable' ' ' # Test that we don't have to pass --no-prompt when difftool.prompt is false -test_expect_success PERL 'difftool.prompt config variable is false' ' +test_expect_success 'difftool.prompt config variable is false' ' difftool_test_setup && test_config difftool.prompt false && echo branch >expect && @@ -221,7 +219,7 @@ test_expect_success PERL 'difftool.prompt config variable is false' ' ' # Test that we don't have to pass --no-prompt when mergetool.prompt is false -test_expect_success PERL 'difftool merge.prompt = false' ' +test_expect_success 'difftool merge.prompt = false' ' difftool_test_setup && test_might_fail git config --unset difftool.prompt && test_config mergetool.prompt false && @@ -231,7 +229,7 @@ test_expect_success PERL 'difftool merge.prompt = false' ' ' # Test that the -y flag can override difftool.prompt = true -test_expect_success PERL 'difftool.prompt can overridden with -y' ' +test_expect_success 'difftool.prompt can overridden with -y' ' difftool_test_setup && test_config difftool.prompt true && echo branch >expect && @@ -240,7 +238,7 @@ test_expect_success PERL 'difftool.prompt can overridden with -y' ' ' # Test that the --prompt flag can override difftool.prompt = false -test_expect_success PERL 'difftool.prompt can overridden with --prompt' ' +test_expect_success 'difftool.prompt can overridden with --prompt' ' difftool_test_setup && test_config difftool.prompt false && echo >input && @@ -250,7 +248,7 @@ test_expect_success PERL 'difftool.prompt can overridden with --prompt' ' ' # Test that the last flag passed on the command-line wins -test_expect_success PERL 'difftool last flag wins' ' +test_expect_success 'difftool last flag wins' ' difftool_test_setup && echo branch >expect && git difftool --prompt --no-prompt branch >actual && @@ -263,7 +261,7 @@ test_expect_success PERL 'difftool last flag wins' ' # git-difftool falls back to git-mergetool config variables # so test that behavior here -test_expect_success PERL 'difftool + mergetool config variables' ' +test_expect_success 'difftool + mergetool config variables' ' test_config merge.tool test-tool && test_config mergetool.test-tool.cmd "cat \$LOCAL" && echo branch >expect && @@ -277,49 +275,49 @@ test_expect_success PERL 'difftool + mergetool config variables' ' test_cmp expect actual ' -test_expect_success PERL 'difftool..path' ' +test_expect_success 'difftool..path' ' test_config difftool.tkdiff.path echo && git difftool --tool=tkdiff --no-prompt branch >output && lines=$(grep file output | wc -l) && test "$lines" -eq 1 ' -test_expect_success PERL 'difftool --extcmd=cat' ' +test_expect_success 'difftool --extcmd=cat' ' echo branch >expect && echo master >>expect && git difftool --no-prompt --extcmd=cat branch >actual && test_cmp expect actual ' -test_expect_success PERL 'difftool --extcmd cat' ' +test_expect_success 'difftool --extcmd cat' ' echo branch >expect && echo master >>expect && git difftool --no-prompt --extcmd=cat branch >actual && test_cmp expect actual ' -test_expect_success PERL 'difftool -x cat' ' +test_expect_success 'difftool -x cat' ' echo branch >expect && echo master >>expect && git difftool --no-prompt -x cat branch >actual && test_cmp expect actual ' -test_expect_success PERL 'difftool --extcmd echo arg1' ' +test_expect_success 'difftool --extcmd echo arg1' ' echo file >expect && git difftool --no-prompt \ --extcmd sh\ -c\ \"echo\ \$1\" branch >actual && test_cmp expect actual ' -test_expect_success PERL 'difftool --extcmd cat arg1' ' +test_expect_success 'difftool --extcmd cat arg1' ' echo master >expect && git difftool --no-prompt \ --extcmd sh\ -c\ \"cat\ \$1\" branch >actual && test_cmp expect actual ' -test_expect_success PERL 'difftool --extcmd cat arg2' ' +test_expect_success 'difftool --extcmd cat arg2' ' echo branch >expect && git difftool --no-prompt \ --extcmd sh\ -c\ \"cat\ \$2\" branch >actual && @@ -327,7 +325,7 @@ test_expect_success PERL 'difftool --extcmd cat arg2' ' ' # Create a second file on master and a different version on branch -test_expect_success PERL 'setup with 2 files different' ' +test_expect_success 'setup with 2 files different' ' echo m2 >file2 && git add file2 && git commit -m "added file2" && @@ -339,7 +337,7 @@ test_expect_success PERL 'setup with 2 files different' ' git checkout master ' -test_expect_success PERL 'say no to the first file' ' +test_expect_success 'say no to the first file' ' (echo n && echo) >input && git difftool -x cat branch output && grep m2 output && @@ -348,7 +346,7 @@ test_expect_success PERL 'say no to the first file' ' ! grep branch output ' -test_expect_success PERL 'say no to the second file' ' +test_expect_success 'say no to the second file' ' (echo && echo n) >input && git difftool -x cat branch output && grep master output && @@ -357,7 +355,7 @@ test_expect_success PERL 'say no to the second file' ' ! grep br2 output ' -test_expect_success PERL 'ending prompt input with EOF' ' +test_expect_success 'ending prompt input with EOF' ' git difftool -x cat branch output && ! grep master output && ! grep branch output && @@ -365,12 +363,12 @@ test_expect_success PERL 'ending prompt input with EOF' ' ! grep br2 output ' -test_expect_success PERL 'difftool --tool-help' ' +test_expect_success 'difftool --tool-help' ' git difftool --tool-help >output && grep tool output ' -test_expect_success PERL 'setup change in subdirectory' ' +test_expect_success 'setup change in subdirectory' ' git checkout master && mkdir sub && echo master >sub/sub && @@ -384,11 +382,11 @@ test_expect_success PERL 'setup change in subdirectory' ' ' run_dir_diff_test () { - test_expect_success PERL "$1 --no-symlinks" " + test_expect_success "$1 --no-symlinks" " symlinks=--no-symlinks && $2 " - test_expect_success PERL,SYMLINKS "$1 --symlinks" " + test_expect_success SYMLINKS "$1 --symlinks" " symlinks=--symlinks && $2 " @@ -510,7 +508,7 @@ do done >actual EOF -test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstaged changes' ' +test_expect_success SYMLINKS 'difftool --dir-diff --symlink without unstaged changes' ' cat >expect <<-EOF && file $PWD/file @@ -547,7 +545,7 @@ write_script modify-file <<\EOF echo "new content" >file EOF -test_expect_success PERL 'difftool --no-symlinks does not overwrite working tree file ' ' +test_expect_success 'difftool --no-symlinks does not overwrite working tree file ' ' echo "orig content" >file && git difftool --dir-diff --no-symlinks --extcmd "$PWD/modify-file" branch && echo "new content" >expect && @@ -560,7 +558,7 @@ echo "tmp content" >"$2/file" && echo "$2" >tmpdir EOF -test_expect_success PERL 'difftool --no-symlinks detects conflict ' ' +test_expect_success 'difftool --no-symlinks detects conflict ' ' ( TMPDIR=$TRASH_DIRECTORY && export TMPDIR && @@ -573,7 +571,7 @@ test_expect_success PERL 'difftool --no-symlinks detects conflict ' ' ) ' -test_expect_success PERL 'difftool properly honors gitlink and core.worktree' ' +test_expect_success 'difftool properly honors gitlink and core.worktree' ' git submodule add ./. submod/ule && test_config -C submod/ule diff.tool checktrees && test_config -C submod/ule difftool.checktrees.cmd '\'' @@ -587,7 +585,7 @@ test_expect_success PERL 'difftool properly honors gitlink and core.worktree' ' ) ' -test_expect_success PERL,SYMLINKS 'difftool --dir-diff symlinked directories' ' +test_expect_success SYMLINKS 'difftool --dir-diff symlinked directories' ' git init dirlinks && ( cd dirlinks && From 231d1903d9a8f5519dfc42aaa0af6fcfe67962f2 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 25 Jan 2017 13:35:42 -0500 Subject: [PATCH 4/7] difftool: hack around -Wzero-length-format warning Building with "gcc -Wall" will complain that the format in: warning("") is empty. Which is true, but the warning is over-eager. We are calling the function for its side effect of printing "warning:", even with an empty string. Our DEVELOPER Makefile knob disables the warning, but not everybody uses it. Let's silence the warning in the code so that nobody reports it or tries to "fix" it. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/difftool.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/difftool.c b/builtin/difftool.c index 42ad9e804a..b5e85ab079 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -567,7 +567,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, warning(_("both files modified: '%s' and '%s'."), wtdir.buf, rdir.buf); warning(_("working tree file has been left.")); - warning(""); + warning("%s", ""); err = 1; } else if (unlink(wtdir.buf) || copy_file(wtdir.buf, rdir.buf, st.st_mode)) From 8c6adc2062f4079e3dff933a5f22f57126f53126 Mon Sep 17 00:00:00 2001 From: David Aguilar Date: Sun, 5 Feb 2017 13:23:38 -0800 Subject: [PATCH 5/7] difftool: fix bug when printing usage "git difftool -h" reports an error: fatal: BUG: setup_git_env called without repository Defer repository setup so that the help option processing happens before the repository is initialized. Add tests to ensure that the basic usage works inside and outside of a repository. Signed-off-by: David Aguilar Acked-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- builtin/difftool.c | 8 ++++---- t/t7800-difftool.sh | 13 +++++++++++++ 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/builtin/difftool.c b/builtin/difftool.c index b5e85ab079..d13350ce83 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -647,10 +647,6 @@ int cmd_difftool(int argc, const char **argv, const char *prefix) OPT_END() }; - /* 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; @@ -661,6 +657,10 @@ int cmd_difftool(int argc, const char **argv, const char *prefix) if (tool_help) return print_tool_help(); + /* 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); + if (use_gui_tool && diff_gui_tool && *diff_gui_tool) setenv("GIT_DIFF_TOOL", diff_gui_tool, 1); else if (difftool_cmd) { diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh index aa0ef02597..21e2ac4ad6 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh @@ -23,6 +23,19 @@ prompt_given () test "$prompt" = "Launch 'test-tool' [Y/n]? branch" } +test_expect_success 'basic usage requires no repo' ' + lines=$(git difftool -h | grep ^usage: | wc -l) && + test "$lines" -eq 1 && + # create a ceiling directory to prevent Git from finding a repo + mkdir -p not/repo && + ceiling="$PWD/not" && + lines=$(cd not/repo && + GIT_CEILING_DIRECTORIES="$ceiling" git difftool -h | + grep ^usage: | wc -l) && + test "$lines" -eq 1 && + rmdir -p not/repo +' + # Create a file on master and change it on branch test_expect_success 'setup' ' echo master >file && From d18fce51a7a7a70774a2cddfaa69149525fb4772 Mon Sep 17 00:00:00 2001 From: David Aguilar Date: Tue, 7 Feb 2017 01:16:59 -0800 Subject: [PATCH 6/7] t7800: simplify basic usage test Use "test_line_count" instead of "wc -l", use "git -C" instead of a subshell, and use test_expect_code when calling difftool. Ease debugging by capturing output into temporary files. Suggested-by: Johannes Schindelin Signed-off-by: David Aguilar Signed-off-by: Junio C Hamano --- t/t7800-difftool.sh | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh index 21e2ac4ad6..97bae54d83 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh @@ -24,16 +24,15 @@ prompt_given () } test_expect_success 'basic usage requires no repo' ' - lines=$(git difftool -h | grep ^usage: | wc -l) && - test "$lines" -eq 1 && + test_expect_code 129 git difftool -h >output && + grep ^usage: output && # create a ceiling directory to prevent Git from finding a repo mkdir -p not/repo && - ceiling="$PWD/not" && - lines=$(cd not/repo && - GIT_CEILING_DIRECTORIES="$ceiling" git difftool -h | - grep ^usage: | wc -l) && - test "$lines" -eq 1 && - rmdir -p not/repo + test_when_finished rm -r not && + test_expect_code 129 \ + env GIT_CEILING_DIRECTORIES="$(pwd)/not" \ + git -C not/repo difftool -h >output && + grep ^usage: output ' # Create a file on master and change it on branch From 7491173c8c0935d283de671552e5e157537d3bd7 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 21 Feb 2017 15:20:15 +0100 Subject: [PATCH 7/7] TEMPORARY: re-add opt-in for builtin difftool Upstream Git already picked up the builtin difftool (it took only six iterations this time to get our changes into upstream) and it will be part of v2.12.0. In the meantime, to make maintenance of Git for Windows easier, let's just add back the opt-in, and then drop this patch as soon as it is time to release Git for Windows v2.12.0. Signed-off-by: Johannes Schindelin --- .gitignore | 1 + Makefile | 1 + builtin/difftool.c | 47 ++++++++++++++++++- ...-difftool.perl => git-legacy-difftool.perl | 0 git.c | 7 ++- t/t7800-difftool.sh | 41 +++++++++++++++- 6 files changed, 93 insertions(+), 4 deletions(-) rename contrib/examples/git-difftool.perl => git-legacy-difftool.perl (100%) mode change 100755 => 100644 diff --git a/.gitignore b/.gitignore index 6722f78f9a..5555ae025b 100644 --- a/.gitignore +++ b/.gitignore @@ -76,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 71a4273f1d..4137a9915b 100644 --- a/Makefile +++ b/Makefile @@ -527,6 +527,7 @@ 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 d13350ce83..a3510caf13 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -616,10 +616,34 @@ 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(&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; + tool_help = 0, nongit; static char *difftool_cmd = NULL, *extcmd = NULL; struct option builtin_difftool_options[] = { OPT_BOOL('g', "gui", &use_gui_tool, @@ -647,6 +671,25 @@ 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_gently(&nongit); + trace_repo_setup(prefix); + if (!nongit) + setup_work_tree(); + git_config(difftool_config, NULL); symlinks = has_symlinks; @@ -657,6 +700,8 @@ int cmd_difftool(int argc, const char **argv, const char *prefix) if (tool_help) return print_tool_help(); + if (nongit) + die("Not a git repository (or any of the parent directories): .git"); /* 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); diff --git a/contrib/examples/git-difftool.perl b/git-legacy-difftool.perl old mode 100755 new mode 100644 similarity index 100% rename from contrib/examples/git-difftool.perl rename to git-legacy-difftool.perl diff --git a/git.c b/git.c index d2d843d824..519a4c6f82 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 97bae54d83..e7ec09d6f3 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh @@ -23,13 +23,41 @@ prompt_given () test "$prompt" = "Launch 'test-tool' [Y/n]? branch" } +for use_builtin_difftool in false true +do + +case "$use_builtin_difftool" in +true) + GIT_CONFIG_PARAMETERS="'difftool.usebuiltin=true'" + export GIT_CONFIG_PARAMETERS + ;; +false) + test_have_prereq PERL || { + say 'Skipping scripted difftool (missing PERL)' + continue + } + ;; +esac + +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 +' + test_expect_success 'basic usage requires no repo' ' - test_expect_code 129 git difftool -h >output && + code='$(test "$use_builtin_difftool" = true && echo 129 || echo 0)' && + test_expect_code $code git difftool -h >output && grep ^usage: output && # create a ceiling directory to prevent Git from finding a repo mkdir -p not/repo && test_when_finished rm -r not && - test_expect_code 129 \ + test_expect_code $code \ env GIT_CEILING_DIRECTORIES="$(pwd)/not" \ git -C not/repo difftool -h >output && grep ^usage: output @@ -616,4 +644,13 @@ test_expect_success 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 +' + +done + test_done