[v2,3/4] difftool: use a strbuf to create the tmpdir path

Commit Message

David Aguilar Sept. 19, 2021, 2:33 a.m. UTC

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

Add a unit test to ensure that double-slashes are not present.

Signed-off-by: David Aguilar <[e-mail]>
---
Changes since v1:
- Updated to use strbuf_trim_trailing_dir_sep().

Thanks Peff!

 builtin/difftool.c  | 28 +++++++++++++++-------------
 t/t7800-difftool.sh |  7 +++++++
 2 files changed, 22 insertions(+), 13 deletions(-)

Comments

Junio C Hamano Sept. 20, 2021, 6:35 p.m. UTC | #1

David Aguilar <[e-mail]> writes:

> Use a strbuf to create the buffer used for the dir-diff tmpdir.
> Strip trailing slashes "/" from the value read from TMPDIR to avoid
> double-slashes in the calculated paths.
>
> Add a unit test to ensure that double-slashes are not present.
>
> Signed-off-by: David Aguilar <[e-mail]>
> ---
> Changes since v1:
> - Updated to use strbuf_trim_trailing_dir_sep().
>
> Thanks Peff!

> @@ -333,11 +332,11 @@ 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)

This is taken hostage by the topic 'ab/retire-option-argument',
which is not yet in 'master' and can never be merged to 'maint',
which is not something you may want to do if this topic is meant as
a bugfix.

I may queue this topic on top of a merge of 'ab/retire-option-argument'
into 'master' for now.

Thanks.

Patch

diff --git a/builtin/difftool.c b/builtin/difftool.c
index 4d2e772031..0554ae5fb5 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -252,11 +252,10 @@  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)
+static NORETURN void exit_cleanup(struct strbuf *buf, int exit_code)
 {
-	struct strbuf buf = STRBUF_INIT;
-	strbuf_addstr(&buf, tmpdir);
-	remove_dir_recursively(&buf, 0);
+	remove_dir_recursively(buf, 0);
+	strbuf_release(buf);
 	if (exit_code)
 		warning(_("failed: %d"), exit_code);
 	exit(exit_code);
@@ -333,11 +332,11 @@  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;
+	struct strbuf tmpdir = STRBUF_INIT;
 	char *lbase_dir, *rbase_dir;
 	size_t ldir_len, rdir_len, wtdir_len;
 	const char *workdir, *tmp;
@@ -360,11 +359,13 @@  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))
+		return error("could not create '%s'", tmpdir.buf);
+	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, '/');
@@ -612,7 +613,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);
@@ -642,11 +643,11 @@  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);
+		exit_cleanup(&tmpdir, rc);
 
 finish:
 	if (fp)
@@ -658,6 +659,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 a923f193da..3863afcaac 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -454,6 +454,13 @@  run_dir_diff_test 'difftool --dir-diff' '
 	grep "^file$" output
 '
 
+run_dir_diff_test 'difftool --dir-diff avoids double-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 &&