From: David Aguilar <davvid@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Git Mailing List" <git@vger.kernel.org>,
"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
"Alan Blotz" <work@blotz.org>,
"Đoàn Trần Công Danh" <congdanhqx@gmail.com>,
"Eric Sunshine" <sunshine@sunshineco.com>,
"Jeff King" <peff@peff.net>
Subject: [PATCH v5] difftool: create a tmpdir path without repeated slashes
Date: Mon, 20 Sep 2021 15:08:55 -0700 [thread overview]
Message-ID: <20210920220855.18637-1-davvid@gmail.com> (raw)
In-Reply-To: <CAPig+cTBfP5_czsPiALcF3tODJmNfXvNkTjqVFRbHCS535d-ng@mail.gmail.com>
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.
Signed-off-by: David Aguilar <davvid@gmail.com>
---
This is a replacement patch for 976bce8a6d (difftool: use a strbuf to
create a tmpdir path without repeated slashes, 2021-09-19) currently
in da/difftool (seen).
Changes since last v4:
- The strbuf leaks noted by Eric have been plugged.
- More variables are initialized so that the finish: section can run without
using unitialized variables.
- Reworked the control flow so that we always "goto finish" on error.
- Got rid of exit_cleanup() entirely -- there's no need for a separate function.
- Reworded the commit message to fold in Ævar's sugs.
builtin/difftool.c | 48 ++++++++++++++++++++++-----------------------
t/t7800-difftool.sh | 7 +++++++
2 files changed, 30 insertions(+), 25 deletions(-)
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 2df6af9e5a..a972431f6d 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,6 +657,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
strbuf_release(&rdir);
strbuf_release(&wtdir);
strbuf_release(&buf);
+ strbuf_release(&tmpdir);
return ret;
}
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 07a52fb8e1..0670b617b4 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 &&
--
2.33.0.372.g520515731c
next prev parent reply other threads:[~2021-09-21 2:20 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-19 20:38 [PATCH v4 0/3] difftool dir-diff symlink bug fix and cleanup patches David Aguilar
2021-09-19 20:38 ` [PATCH v4 1/3] difftool: fix symlink-file writing in dir-diff mode David Aguilar
2021-09-20 7:59 ` Ævar Arnfjörð Bjarmason
2021-09-19 20:38 ` [PATCH v4 2/3] difftool: use a strbuf to create a tmpdir path without repeated slashes David Aguilar
2021-09-20 5:40 ` Eric Sunshine
2021-09-20 22:08 ` David Aguilar [this message]
2021-09-19 20:38 ` [PATCH v4 3/3] difftool: add a missing space to the run_dir_diff() comments David Aguilar
2021-09-20 18:39 ` [PATCH v4 0/3] difftool dir-diff symlink bug fix and cleanup patches Junio C Hamano
2021-09-20 21:43 ` David Aguilar
2021-09-22 18:43 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210920220855.18637-1-davvid@gmail.com \
--to=davvid@gmail.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=congdanhqx@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=peff@peff.net \
--cc=sunshine@sunshineco.com \
--cc=work@blotz.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).