[PATCH v4 0/3] difftool dir-diff symlink bug fix and cleanup patches

STALE1838d

Revision v4 of 2 in this series.

7 messages, 3 authors, 2021-09-21 · open the first message on its own page

[PATCH v4 0/3] difftool dir-diff symlink bug fix and cleanup patches

From: David Aguilar <hidden>
Date: 2021-09-19 20:38:39

This patch series fixes a regression in difftool that can lead to data loss.
The symlink-file writing in difftool's dir-diff mode has been fixed to no
longer write-through to the symlink targets.

Please consider patching older Git versions with the fix from 1/3.

Changes since last time:
- This series has been reordered so that 1/3 is the patch that fixes the bug.
  The subsequent patches are cleanup.
- Patch 1/4 from before, which removed the tmp test repos, has been dropped.
- Patch 1/3 was updated to not remove its tmp test repo.
- Patch 1/3 was updated to consistently use "echo" in its tests.
- Patch 1/3 was updated to fix a "syminks" -> "symlinks" test comment typo.
- Patch 2/3 was reworded to improve the repeated slashes justification.
- Patch 3/3 "add a missing space to the ... comments" is unchanged.

David Aguilar (3):
  difftool: fix symlink-file writing in dir-diff mode
  difftool: use a strbuf to create a tmpdir path without double-slashes
  difftool: add a missing space to the run_dir_diff() comments

 builtin/difftool.c  | 32 ++++++++++---------
 t/t7800-difftool.sh | 75 +++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 91 insertions(+), 16 deletions(-)

-- 
2.33.0.720.g5b0b3ce580

[PATCH v4 1/3] difftool: fix symlink-file writing in dir-diff mode

From: David Aguilar <hidden>
Date: 2021-09-19 20:38:40

The difftool dir-diff mode handles symlinks by replacing them with their
readlink(2) values. This allows diff tools to see changes to symlinks
as if they were regular text diffs with the old and new path values.
This is analogous to what "git diff" displays when symlinks change.

The temporary diff directories that are created initially contain
symlinks because they get checked-out using a temporary index that
retains the original symlinks as checked-in to the repository.

A bug was introduced when difftool was rewritten in C that made
difftool write the readlink(2) contents into the pointed-to file rather
than the symlink itself. The write was going through the symlink and
writing to its target rather than writing to the symlink path itself.

Replace symlinks with raw text files by unlinking the symlink path
before writing the readlink(2) content into them.

When 18ec800512eb0634a0bf5e86b36ed156fbee73f3 added handling for
modified symlinks this bug got recorded in the test suite. The tests
included the pointed-to symlink target paths. These paths were being
reported because difftool was erroneously writing to them, but they
should have never been reported nor written.

Correct the modified-symlinks test cases by removing the target files
from the expected output.

Add a test to ensure that symlinks are written with the readlink(2)
values and that the target files contain their original content.

Reported-by: Alan Blotz <redacted>
Helped-by: Đoàn Trần Công Danh [off-list ref]
Signed-off-by: David Aguilar <redacted>
---
 builtin/difftool.c  |  2 ++
 t/t7800-difftool.sh | 68 +++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 68 insertions(+), 2 deletions(-)
diff --git a/builtin/difftool.c b/builtin/difftool.c
index bb9fe7245a..21e055d13a 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -557,11 +557,13 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 		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);
 		}
 	}
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index a173f564bc..07a52fb8e1 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -674,7 +674,6 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '
 	rm c &&
 	ln -s d c &&
 	cat >expect <<-EOF &&
-		b
 		c
 
 		c
@@ -710,7 +709,6 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '
 	# Deleted symlinks
 	rm -f c &&
 	cat >expect <<-EOF &&
-		b
 		c
 
 	EOF
@@ -723,6 +721,72 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '
 	test_cmp expect actual
 '
 
+test_expect_success SYMLINKS 'difftool --dir-diff writes symlinks as raw text' '
+	# Start out on a branch called "branch-init".
+	git init -b branch-init symlink-files &&
+	(
+		cd ./symlink-files &&
+
+		# This test ensures that symlinks are written as raw text.
+		# The "cat" tools output link and file contents.
+		git config difftool.cat-left-link.cmd "cat \"\$LOCAL/link\"" &&
+		git config difftool.cat-left-a.cmd "cat \"\$LOCAL/file-a\"" &&
+		git config difftool.cat-right-link.cmd "cat \"\$REMOTE/link\"" &&
+		git config difftool.cat-right-b.cmd "cat \"\$REMOTE/file-b\"" &&
+
+		# Record the empty initial state so that we can come back here
+		# later and not have to consider the any cases where difftool
+		# will create symlinks back into the worktree.
+		test_tick &&
+		git commit --allow-empty -m init &&
+
+		# Create a file called "file-a" with a symlink pointing to it.
+		git switch -c branch-a &&
+		echo a >file-a &&
+		ln -s file-a link &&
+		git add file-a link &&
+		test_tick &&
+		git commit -m link-to-file-a &&
+
+		# Create a file called "file-b" and point the symlink to it.
+		git switch -c branch-b &&
+		echo b >file-b &&
+		rm link &&
+		ln -s file-b link &&
+		git add file-b link &&
+		git rm file-a &&
+		test_tick &&
+		git commit -m link-to-file-b &&
+
+		# Checkout the initial branch so that the --symlinks behavior is
+		# not activated. The two directories should be completely
+		# independent with no symlinks pointing back here.
+		git switch branch-init &&
+
+		# The left link must be "file-a" and "file-a" must contain "a".
+		echo file-a >expect &&
+		git difftool --symlinks --dir-diff --tool cat-left-link \
+			branch-a branch-b >actual &&
+		test_cmp expect actual &&
+
+		echo a >expect &&
+		git difftool --symlinks --dir-diff --tool cat-left-a \
+			branch-a branch-b >actual &&
+		test_cmp expect actual &&
+
+		# The right link must be "file-b" and "file-b" must contain "b".
+		echo file-b >expect &&
+		git difftool --symlinks --dir-diff --tool cat-right-link \
+			branch-a branch-b >actual &&
+		test_cmp expect actual &&
+
+		echo b >expect &&
+		git difftool --symlinks --dir-diff --tool cat-right-b \
+			branch-a branch-b >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_expect_success 'add -N and difftool -d' '
 	test_when_finished git reset --hard &&
 
-- 
2.33.0.720.g5b0b3ce580

[PATCH v4 2/3] difftool: use a strbuf to create a tmpdir path without repeated slashes

From: David Aguilar <hidden>
Date: 2021-09-19 20:38:47

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.

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

Signed-off-by: David Aguilar <redacted>
---
 builtin/difftool.c  | 28 +++++++++++++++-------------
 t/t7800-difftool.sh |  7 +++++++
 2 files changed, 22 insertions(+), 13 deletions(-)
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 21e055d13a..daa438be09 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, '/');
@@ -614,7 +615,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 +645,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)
@@ -660,6 +661,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.720.g5b0b3ce580

[PATCH v4 3/3] difftool: add a missing space to the run_dir_diff() comments

From: David Aguilar <hidden>
Date: 2021-09-19 20:38:48

Signed-off-by: David Aguilar <redacted>
---
 builtin/difftool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/difftool.c b/builtin/difftool.c
index daa438be09..e225974a2b 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -549,7 +549,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.
 	 */
-- 
2.33.0.720.g5b0b3ce580

Re: [PATCH v4 2/3] difftool: use a strbuf to create a tmpdir path without repeated slashes

From: Eric Sunshine <hidden>
Date: 2021-09-20 05:41:02

On Sun, Sep 19, 2021 at 4:38 PM David Aguilar [off-list ref] wrote:
difftool: use a strbuf to 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.
Mentioning strbuf in the commit message misleads the reviewer into
thinking that it somehow merits extra attention and close scrutiny. In
fact, the opposite is true: strbuf is just an implementation detail;
there is no reason to mention it in the commit message at all. The
commit message's emphasis on strbuf distracts the reader from the real
purpose of this change, which is to fix a cosmetic issue (and maybe a
real issue on Windows in which double-slash can have significance?).
So, perhaps:

    difftool: fold out repeated slashes from TMPDIR

    Paths generated by difftool are passed to user-facing diff tools.
    Supplying paths with repeated slashes is a cosmetic blemish that
    is exposed to users and can be avoided. Therefore, strip trailing
    slashes from the value of TMPDIR to avoid repeated slashes in the
    generated paths.
Add a unit test to ensure that repeated slashes are not present.
Unless there is something unusual or tricky about the new test that
requires extra explanation in the commit message, there is little
reason to mention that you're adding a new test, so I'd probably drop
this line, as well. After all, the patch easily speaks for itself, and
a reviewer can see at a glance that you're adding a new test.
quoted hunk
diff --git a/builtin/difftool.c b/builtin/difftool.c
@@ -252,11 +252,10 @@ static void changed_files(struct hashmap *result, const char *index_path,
-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);
It feels wrong to be releasing the caller-supplied strbuf; this change
makes it harder to reason about ownership. Normally, the entity which
allocates a resource should be the one to release it. More on this
below...
quoted hunk
@@ -333,11 +332,11 @@ static int checkout_path(unsigned mode, struct object_id *oid,
+       struct strbuf tmpdir = STRBUF_INIT;
+       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);
Leaking the `tmpdir` strbuf here. You'd want:

    if (!mkdtemp(tmpdir.buf)) {
        error("could not create '%s'", tmpdir.buf);
        strbuf_release(&tmpdir);
        return -1;
    }
quoted hunk
@@ -644,11 +645,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);
... [continued from above]

Both branches in this conditional terminate the program either
directly by `exit(1)` or indirectly through `exit_cleanup(...)`. Yet
only the `exit_cleanup(...)` branch releases the strbuf (because you
updated `exit_cleanup()` above to do so); the other branch just leaks
the strbuf. This is inconsistent.

Since we're exiting anyhow, and since `exit_cleanup()` was already
leaking its own strbuf even before this patch, and since it feels
somewhat dirty to have `exit_cleanup()` responsible for releasing a
resource it didn't allocate, it may make sense just to maintain the
status-quo and just leak the strbuf before exiting. That is, don't
make any changes to `exit_cleanup()`, and let both of these branches
leak the strbuf.

On the other hand, if you really do want to release the strbuf, then
it would be more consistent for both branches in this conditional to
do so, not just one. That is, add a strbuf_release() to the `then`
arm.
quoted hunk
 finish:
        if (fp)
@@ -660,6 +661,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);
Correctly releasing the strbuf. Good.

Re: [PATCH v4 1/3] difftool: fix symlink-file writing in dir-diff mode

From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-09-20 08:03:11

On Sun, Sep 19 2021, David Aguilar wrote:
The difftool dir-diff mode handles symlinks by replacing them with their
readlink(2) values. This allows diff tools to see changes to symlinks
as if they were regular text diffs with the old and new path values.
This is analogous to what "git diff" displays when symlinks change.

The temporary diff directories that are created initially contain
symlinks because they get checked-out using a temporary index that
retains the original symlinks as checked-in to the repository.

A bug was introduced when difftool was rewritten in C that made
difftool write the readlink(2) contents into the pointed-to file rather
than the symlink itself. The write was going through the symlink and
writing to its target rather than writing to the symlink path itself.

Replace symlinks with raw text files by unlinking the symlink path
before writing the readlink(2) content into them.

When 18ec800512eb0634a0bf5e86b36ed156fbee73f3 added handling for
Nit: Please format commits like this: 18ec800512e (difftool: handle
modified symlinks in dir-diff mode, 2017-03-15)

See "git show -s --pretty=reference" in Documentation/SubmittingPatches.
quoted hunk
modified symlinks this bug got recorded in the test suite. The tests
included the pointed-to symlink target paths. These paths were being
reported because difftool was erroneously writing to them, but they
should have never been reported nor written.

Correct the modified-symlinks test cases by removing the target files
from the expected output.

Add a test to ensure that symlinks are written with the readlink(2)
values and that the target files contain their original content.

Reported-by: Alan Blotz <redacted>
Helped-by: Đoàn Trần Công Danh [off-list ref]
Signed-off-by: David Aguilar <redacted>
---
 builtin/difftool.c  |  2 ++
 t/t7800-difftool.sh | 68 +++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 68 insertions(+), 2 deletions(-)
diff --git a/builtin/difftool.c b/builtin/difftool.c
index bb9fe7245a..21e055d13a 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -557,11 +557,13 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 		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);
 		}
 	}
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index a173f564bc..07a52fb8e1 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -674,7 +674,6 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '
 	rm c &&
 	ln -s d c &&
 	cat >expect <<-EOF &&
-		b
 		c
 
 		c
@@ -710,7 +709,6 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '
 	# Deleted symlinks
 	rm -f c &&
 	cat >expect <<-EOF &&
-		b
 		c
 
 	EOF
@@ -723,6 +721,72 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '
 	test_cmp expect actual
 '
 
+test_expect_success SYMLINKS 'difftool --dir-diff writes symlinks as raw text' '
+	# Start out on a branch called "branch-init".
+	git init -b branch-init symlink-files &&
+	(
+		cd ./symlink-files &&
The "./" isn't needed

Neither of those small comments needs a re-roll I think, just reading
through & noting things I spotted...

I've omitted things that Eric mentioned in
[off-list ref].

[PATCH v5] difftool: create a tmpdir path without repeated slashes

From: David Aguilar <hidden>
Date: 2021-09-21 02:20:10

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 <redacted>
---

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help