difftool: create a tmpdir path without repeated slashes

The paths generated by difftool are passed to user-facing diff tools.
Using paths with repeated slashes in them is a cosmetic blemish that
is exposed to users and can be avoided.

Use a strbuf to create the buffer used for the dir-diff tmpdir.
Strip trailing slashes from the value read from TMPDIR to avoid
repeated slashes in the generated paths.

Adjust the error handling to avoid leaking strbufs and to avoid
returning -1 to cmd_main().

Signed-off-by: David Aguilar <davvid@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/difftool.c b/builtin/difftool.c
index 21e055d13a0a65698743228d7de6edb78a28cda9..797161adcda0e65323cc0050c77ba74739ff36b0 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)) {
@@ -333,16 +323,16 @@ static int checkout_path(unsigned mode, struct object_id *oid,
 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 +341,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 +350,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, '/');
@@ -580,7 +574,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 +608,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 +638,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 +657,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 &&