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>
David Aguilar committed
Sep 30, 2021 at 18:37 UTC
4ac9f15492b69ef3a0334ca66b1ef4be00c79b47
2 files changed
+31
-26
builtin/difftool.c
+24
-26
@@ -252,16 +252,6 @@ static void changed_files(struct hashmap *result, const char *index_path,
252
strbuf_release(&buf);
253
}
254
255
-static NORETURN void exit_cleanup(const char *tmpdir, int exit_code)
256
-{
257
- struct strbuf buf = STRBUF_INIT;
258
- strbuf_addstr(&buf, tmpdir);
259
- remove_dir_recursively(&buf, 0);
260
- if (exit_code)
261
- warning(_("failed: %d"), exit_code);
262
- exit(exit_code);
263
-}
264
-
255
static int ensure_leading_directories(char *path)
256
{
257
switch (safe_create_leading_directories(path)) {
@@ -333,16 +323,16 @@ static int checkout_path(unsigned mode, struct object_id *oid,
323
static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
324
struct child_process *child)
325
{
336
- char tmpdir[PATH_MAX];
326
struct strbuf info = STRBUF_INIT, lpath = STRBUF_INIT;
327
struct strbuf rpath = STRBUF_INIT, buf = STRBUF_INIT;
328
struct strbuf ldir = STRBUF_INIT, rdir = STRBUF_INIT;
329
struct strbuf wtdir = STRBUF_INIT;
341
- char *lbase_dir, *rbase_dir;
330
+ struct strbuf tmpdir = STRBUF_INIT;
331
+ char *lbase_dir = NULL, *rbase_dir = NULL;
332
size_t ldir_len, rdir_len, wtdir_len;
333
const char *workdir, *tmp;
334
int ret = 0, i;
345
- FILE *fp;
335
+ FILE *fp = NULL;
336
struct hashmap working_tree_dups = HASHMAP_INIT(working_tree_entry_cmp,
337
NULL);
338
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,
341
struct pair_entry *entry;
342
struct index_state wtindex;
343
struct checkout lstate, rstate;
354
- int rc, flags = RUN_GIT_CMD, err = 0;
344
+ int flags = RUN_GIT_CMD, err = 0;
345
const char *helper_argv[] = { "difftool--helper", NULL, NULL, NULL };
346
struct hashmap wt_modified, tmp_modified;
347
int indices_loaded = 0;
@@ -360,11 +350,15 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
350
351
/* Setup temp directories */
352
tmp = getenv("TMPDIR");
363
- xsnprintf(tmpdir, sizeof(tmpdir), "%s/git-difftool.XXXXXX", tmp ? tmp : "/tmp");
364
- if (!mkdtemp(tmpdir))
365
- return error("could not create '%s'", tmpdir);
366
- strbuf_addf(&ldir, "%s/left/", tmpdir);
367
- strbuf_addf(&rdir, "%s/right/", tmpdir);
353
+ strbuf_add_absolute_path(&tmpdir, tmp ? tmp : "/tmp");
354
+ strbuf_trim_trailing_dir_sep(&tmpdir);
355
+ strbuf_addstr(&tmpdir, "/git-difftool.XXXXXX");
356
+ if (!mkdtemp(tmpdir.buf)) {
357
+ ret = error("could not create '%s'", tmpdir.buf);
358
+ goto finish;
359
+ }
360
+ strbuf_addf(&ldir, "%s/left/", tmpdir.buf);
361
+ strbuf_addf(&rdir, "%s/right/", tmpdir.buf);
362
strbuf_addstr(&wtdir, workdir);
363
if (!wtdir.len || !is_dir_sep(wtdir.buf[wtdir.len - 1]))
364
strbuf_addch(&wtdir, '/');
@@ -580,7 +574,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
574
flags = 0;
575
} else
576
setenv("GIT_DIFFTOOL_DIRDIFF", "true", 1);
583
- rc = run_command_v_opt(helper_argv, flags);
577
+ ret = run_command_v_opt(helper_argv, flags);
578
579
/* TODO: audit for interaction with sparse-index. */
580
ensure_full_index(&wtindex);
@@ -614,7 +608,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
608
if (!indices_loaded) {
609
struct lock_file lock = LOCK_INIT;
610
strbuf_reset(&buf);
617
- strbuf_addf(&buf, "%s/wtindex", tmpdir);
611
+ strbuf_addf(&buf, "%s/wtindex", tmpdir.buf);
612
if (hold_lock_file_for_update(&lock, buf.buf, 0) < 0 ||
613
write_locked_index(&wtindex, &lock, COMMIT_LOCK)) {
614
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,
638
}
639
640
if (err) {
647
- warning(_("temporary files exist in '%s'."), tmpdir);
641
+ warning(_("temporary files exist in '%s'."), tmpdir.buf);
642
warning(_("you may want to cleanup or recover these."));
649
- exit(1);
650
- } else
651
- exit_cleanup(tmpdir, rc);
643
+ ret = 1;
644
+ } else {
645
+ remove_dir_recursively(&tmpdir, 0);
646
+ if (ret)
647
+ warning(_("failed: %d"), ret);
648
+ }
649
650
finish:
651
if (fp)
@@ -660,8 +657,9 @@ finish:
657
strbuf_release(&rdir);
658
strbuf_release(&wtdir);
659
strbuf_release(&buf);
660
+ strbuf_release(&tmpdir);
661
664
- return ret;
662
+ return (ret < 0) ? 1 : ret;
663
}
664
665
static int run_file_diff(int prompt, const char *prefix,
t/t7800-difftool.sh
+7
@@ -453,6 +453,13 @@ run_dir_diff_test 'difftool --dir-diff' '
453
grep "^file$" output
454
'
455
456
+run_dir_diff_test 'difftool --dir-diff avoids repeated slashes in TMPDIR' '
457
+ TMPDIR="${TMPDIR:-/tmp}////" \
458
+ git difftool --dir-diff $symlinks --extcmd echo branch >output &&
459
+ grep -v // output >actual &&
460
+ test_line_count = 1 actual
461
+'
462
+
463
run_dir_diff_test 'difftool --dir-diff ignores --prompt' '
464
git difftool --dir-diff $symlinks --prompt --extcmd ls branch >output &&
465
grep "^sub$" output &&