Merge branch 'da/difftool'

Code clean-up in "git difftool".

* da/difftool:
  difftool: add a missing space to the run_dir_diff() comments
  difftool: remove an unnecessary call to strbuf_release()
  difftool: refactor dir-diff to write files using helper functions
  difftool: create a tmpdir path without repeated slashes

diff --git a/builtin/difftool.c b/builtin/difftool.c
index 210da03908c7ccd1cb041d8c3af7c09ba3c0baed..4931c108451721ebd49a3009adb431dbe41eb662 100644 (file)
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -252,16 +252,6 @@ static void changed_files(struct hashmap *result, const char *index_path,
        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)) {
@@ -330,19 +320,44 @@ static int checkout_path(unsigned mode, struct object_id *oid,
        return ret;
 }
 
+static void write_file_in_directory(struct strbuf *dir, size_t dir_len,
+                       const char *path, const char *content)
+{
+       add_path(dir, dir_len, path);
+       ensure_leading_directories(dir->buf);
+       unlink(dir->buf);
+       write_file(dir->buf, "%s", content);
+}
+
+/* Write the file contents for the left and right sides of the difftool
+ * dir-diff representation for submodules and symlinks. Symlinks and submodules
+ * are written as regular text files so that external diff tools can diff them
+ * as text files, resulting in behavior that is analogous to to what "git diff"
+ * displays for symlink and submodule diffs.
+ */
+static void write_standin_files(struct pair_entry *entry,
+                       struct strbuf *ldir, size_t ldir_len,
+                       struct strbuf *rdir, size_t rdir_len)
+{
+       if (*entry->left)
+               write_file_in_directory(ldir, ldir_len, entry->path, entry->left);
+       if (*entry->right)
+               write_file_in_directory(rdir, rdir_len, entry->path, entry->right);
+}
+
 static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
                        struct child_process *child)
 {
-       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;
-       char *lbase_dir, *rbase_dir;
+       struct strbuf tmpdir = STRBUF_INIT;
+       char *lbase_dir = NULL, *rbase_dir = NULL;
        size_t ldir_len, rdir_len, wtdir_len;
        const char *workdir, *tmp;
        int ret = 0, i;
-       FILE *fp;
+       FILE *fp = NULL;
        struct hashmap working_tree_dups = HASHMAP_INIT(working_tree_entry_cmp,
                                                        NULL);
        struct hashmap submodules = HASHMAP_INIT(pair_cmp, NULL);
@@ -351,7 +366,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
        struct pair_entry *entry;
        struct index_state wtindex;
        struct checkout lstate, rstate;
-       int rc, flags = RUN_GIT_CMD, err = 0;
+       int flags = RUN_GIT_CMD, err = 0;
        const char *helper_argv[] = { "difftool--helper", NULL, NULL, NULL };
        struct hashmap wt_modified, tmp_modified;
        int indices_loaded = 0;
@@ -360,11 +375,15 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 
        /* 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_add_absolute_path(&tmpdir, tmp ? tmp : "/tmp");
+       strbuf_trim_trailing_dir_sep(&tmpdir);
+       strbuf_addstr(&tmpdir, "/git-difftool.XXXXXX");
+       if (!mkdtemp(tmpdir.buf)) {
+               ret = error("could not create '%s'", tmpdir.buf);
+               goto finish;
+       }
+       strbuf_addf(&ldir, "%s/left/", tmpdir.buf);
+       strbuf_addf(&rdir, "%s/right/", tmpdir.buf);
        strbuf_addstr(&wtdir, workdir);
        if (!wtdir.len || !is_dir_sep(wtdir.buf[wtdir.len - 1]))
                strbuf_addch(&wtdir, '/');
@@ -535,40 +554,19 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
         */
        hashmap_for_each_entry(&submodules, &iter, entry,
                                entry /* member name */) {
-               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);
-               }
+               write_standin_files(entry, &ldir, ldir_len, &rdir, rdir_len);
        }
 
        /*
-        * Symbolic links require special treatment.The standard "git diff"
+        * 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_for_each_entry(&symlinks2, &iter, entry,
                                entry /* member name */) {
-               if (*entry->left) {
-                       add_path(&ldir, ldir_len, entry->path);
-                       ensure_leading_directories(ldir.buf);
-                       unlink(ldir.buf);
-                       write_file(ldir.buf, "%s", entry->left);
-               }
-               if (*entry->right) {
-                       add_path(&rdir, rdir_len, entry->path);
-                       ensure_leading_directories(rdir.buf);
-                       unlink(rdir.buf);
-                       write_file(rdir.buf, "%s", entry->right);
-               }
-       }
 
-       strbuf_release(&buf);
+               write_standin_files(entry, &ldir, ldir_len, &rdir, rdir_len);
+       }
 
        strbuf_setlen(&ldir, ldir_len);
        helper_argv[1] = ldir.buf;
@@ -580,7 +578,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
                flags = 0;
        } else
                setenv("GIT_DIFFTOOL_DIRDIFF", "true", 1);
-       rc = run_command_v_opt(helper_argv, flags);
+       ret = run_command_v_opt(helper_argv, flags);
 
        /* TODO: audit for interaction with sparse-index. */
        ensure_full_index(&wtindex);
@@ -614,7 +612,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
                if (!indices_loaded) {
                        struct lock_file lock = LOCK_INIT;
                        strbuf_reset(&buf);
-                       strbuf_addf(&buf, "%s/wtindex", tmpdir);
+                       strbuf_addf(&buf, "%s/wtindex", tmpdir.buf);
                        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);
@@ -644,11 +642,14 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
        }
 
        if (err) {
-               warning(_("temporary files exist in '%s'."), tmpdir);
+               warning(_("temporary files exist in '%s'."), tmpdir.buf);
                warning(_("you may want to cleanup or recover these."));
-               exit(1);
-       } else
-               exit_cleanup(tmpdir, rc);
+               ret = 1;
+       } else {
+               remove_dir_recursively(&tmpdir, 0);
+               if (ret)
+                       warning(_("failed: %d"), ret);
+       }
 
 finish:
        if (fp)
@@ -660,8 +661,9 @@ finish:
        strbuf_release(&rdir);
        strbuf_release(&wtdir);
        strbuf_release(&buf);
+       strbuf_release(&tmpdir);
 
-       return ret;
+       return (ret < 0) ? 1 : ret;
 }
 
 static int run_file_diff(int prompt, const char *prefix,

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 528e0dabf08653b73f2bc9ced333d4c79fa70daf..096456292c0ad604d90c1d4d2aebb8aacf883d99 100755 (executable)
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -453,6 +453,13 @@ run_dir_diff_test 'difftool --dir-diff' '
        grep "^file$" output
 '
 
+run_dir_diff_test 'difftool --dir-diff avoids repeated slashes in TMPDIR' '
+       TMPDIR="${TMPDIR:-/tmp}////" \
+               git difftool --dir-diff $symlinks --extcmd echo branch >output &&
+       grep -v // output >actual &&
+       test_line_count = 1 actual
+'
+
 run_dir_diff_test 'difftool --dir-diff ignores --prompt' '
        git difftool --dir-diff $symlinks --prompt --extcmd ls branch >output &&
        grep "^sub$" output &&