From 4ac9f15492b69ef3a0334ca66b1ef4be00c79b47 Mon Sep 17 00:00:00 2001 From: David Aguilar Date: Thu, 30 Sep 2021 18:37:53 -0700 Subject: [PATCH 1/4] 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 Signed-off-by: Junio C Hamano --- builtin/difftool.c | 50 ++++++++++++++++++++++----------------------- t/t7800-difftool.sh | 7 +++++++ 2 files changed, 31 insertions(+), 26 deletions(-) diff --git a/builtin/difftool.c b/builtin/difftool.c index 21e055d13a..797161adcd 100644 --- 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 528e0dabf0..096456292c 100755 --- 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 && From 2255c80c91da9ed7fa127dad3aa92eb9f448c88b Mon Sep 17 00:00:00 2001 From: David Aguilar Date: Thu, 30 Sep 2021 18:37:54 -0700 Subject: [PATCH 2/4] difftool: refactor dir-diff to write files using helper functions Add a helpers function to handle the unlinking and writing of the dir-diff submodule and symlink stand-in files. Use the helpers to implement the guts of the hashmap loops. This eliminate duplicate code and safeguards the submodules hashmap loop against the symlink-chasing behavior that 5bafb3576a (difftool: fix symlink-file writing in dir-diff mode, 2021-09-22) addressed. The submodules loop should not strictly require the unlink() call that this is introducing to them, but it does not necessarily hurt them either beyond the cost of the extra unlink(). Signed-off-by: David Aguilar Signed-off-by: Junio C Hamano --- builtin/difftool.c | 50 ++++++++++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 22 deletions(-) diff --git a/builtin/difftool.c b/builtin/difftool.c index 797161adcd..be7ed2acaa 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -320,6 +320,31 @@ 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) { @@ -529,16 +554,7 @@ 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); } /* @@ -548,18 +564,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, */ 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); - } + + write_standin_files(entry, &ldir, ldir_len, &rdir, rdir_len); } strbuf_release(&buf); From 8e2af8f0db329a8c0211a309fa9cb611b6dc2517 Mon Sep 17 00:00:00 2001 From: David Aguilar Date: Thu, 30 Sep 2021 18:37:55 -0700 Subject: [PATCH 3/4] difftool: remove an unnecessary call to strbuf_release() The `buf` strbuf is reused again later in the same function, so there is no benefit to calling strbuf_release(). The subsequent usage is already using strbuf_reset() to reset the buffer, so releasing it early is only going to lead to a wasteful reallocation. Remove the early call to strbuf_release(). The same strbuf is already cleaned up in the "finish:" section so nothing is leaked, either. Signed-off-by: David Aguilar Signed-off-by: Junio C Hamano --- builtin/difftool.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/builtin/difftool.c b/builtin/difftool.c index be7ed2acaa..1a415a15a3 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -568,8 +568,6 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, write_standin_files(entry, &ldir, ldir_len, &rdir, rdir_len); } - strbuf_release(&buf); - strbuf_setlen(&ldir, ldir_len); helper_argv[1] = ldir.buf; strbuf_setlen(&rdir, rdir_len); From 28c10ecbfcd68062da602de98702bab71d76b12b Mon Sep 17 00:00:00 2001 From: David Aguilar Date: Thu, 30 Sep 2021 18:37:56 -0700 Subject: [PATCH 4/4] difftool: add a missing space to the run_dir_diff() comments Signed-off-by: David Aguilar 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 1a415a15a3..a9d6553f72 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -558,7 +558,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, } /* - * 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. */